From b79152dfaa29dba5cc10e525bdfc7ccc8424dc9d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 30 Apr 2026 01:15:31 +0000 Subject: [PATCH 1/4] Initial plan From 6f4294b74770d0dc27adfa036fc7d151ebe97fd8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 30 Apr 2026 01:40:41 +0000 Subject: [PATCH 2/4] feat: parameterize safe-output PR policy fields in workflow_call workflows Agent-Logs-Url: https://github.com/github/gh-aw/sessions/f61f1ac7-d8e7-4154-8d64-f3d6642ef313 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../setup/js/manifest_file_helpers.test.cjs | 20 +++ actions/setup/js/safe_outputs_handlers.cjs | 34 +++++ .../setup/js/safe_outputs_handlers.test.cjs | 46 +++++++ .../reference/safe-outputs-pull-requests.md | 44 +++++++ pkg/parser/schema_test.go | 48 +++++++ pkg/parser/schemas/main_workflow_schema.json | 84 +++++++++--- .../compiler_safe_outputs_config_test.go | 88 +++++++++++++ pkg/workflow/validation_helpers.go | 20 ++- pkg/workflow/validation_helpers_test.go | 121 ++++++++++++++++++ 9 files changed, 486 insertions(+), 19 deletions(-) diff --git a/actions/setup/js/manifest_file_helpers.test.cjs b/actions/setup/js/manifest_file_helpers.test.cjs index 5c8afe8f2a8..3f4d837ca19 100644 --- a/actions/setup/js/manifest_file_helpers.test.cjs +++ b/actions/setup/js/manifest_file_helpers.test.cjs @@ -545,5 +545,25 @@ index abc..def 100644 expect(result.action).toBe("deny"); expect(result.source).toBe("allowlist"); }); + + it("should fail closed (deny) when protected_files_policy resolves to an invalid value", () => { + // An expression like ${{ inputs.policy }} that resolves to an unrecognised string + // must not silently allow protected file changes — it must deny (fail closed). + const result = checkFileProtection(makePatch("package.json"), { + protected_files: ["package.json"], + protected_files_policy: "not-a-valid-policy", + }); + expect(result.action).toBe("deny"); + expect(result.source).toBe("protected"); + }); + + it("should fail closed (deny) when protected_files_policy is undefined", () => { + const result = checkFileProtection(makePatch("package.json"), { + protected_files: ["package.json"], + // protected_files_policy intentionally absent + }); + expect(result.action).toBe("deny"); + expect(result.source).toBe("protected"); + }); }); }); diff --git a/actions/setup/js/safe_outputs_handlers.cjs b/actions/setup/js/safe_outputs_handlers.cjs index 037fb61c202..d5d015f72e1 100644 --- a/actions/setup/js/safe_outputs_handlers.cjs +++ b/actions/setup/js/safe_outputs_handlers.cjs @@ -320,6 +320,23 @@ function createHandlers(server, appendSafeOutput, config = {}) { // Determine transport format: "bundle" uses git bundle (preserves merge topology), // "am" (default) uses git format-patch / git am (good for linear histories). const patchFormat = prConfig["patch_format"] || config["patch_format"] || "am"; + const validPatchFormats = ["am", "bundle"]; + if (!validPatchFormats.includes(patchFormat)) { + const errorMsg = `Invalid patch_format: "${patchFormat}". Must be one of: ${validPatchFormats.join(", ")}`; + server.debug(`create_pull_request: ${errorMsg}`); + return { + content: [ + { + type: "text", + text: JSON.stringify({ + result: "error", + error: errorMsg, + }), + }, + ], + isError: true, + }; + } const useBundle = patchFormat === "bundle"; // Build common options for both patch and bundle generation @@ -531,6 +548,23 @@ function createHandlers(server, appendSafeOutput, config = {}) { // Determine transport format: "bundle" uses git bundle (preserves merge topology), // "am" (default) uses git format-patch / git am (good for linear histories). const pushPatchFormat = pushConfig["patch_format"] || config["patch_format"] || "am"; + const validPushPatchFormats = ["am", "bundle"]; + if (!validPushPatchFormats.includes(pushPatchFormat)) { + const errorMsg = `Invalid patch_format: "${pushPatchFormat}". Must be one of: ${validPushPatchFormats.join(", ")}`; + server.debug(`push_to_pull_request_branch: ${errorMsg}`); + return { + content: [ + { + type: "text", + text: JSON.stringify({ + result: "error", + error: errorMsg, + }), + }, + ], + isError: true, + }; + } const useBundle = pushPatchFormat === "bundle"; // Build common options for both patch and bundle generation diff --git a/actions/setup/js/safe_outputs_handlers.test.cjs b/actions/setup/js/safe_outputs_handlers.test.cjs index 09332830b67..7d845354fdb 100644 --- a/actions/setup/js/safe_outputs_handlers.test.cjs +++ b/actions/setup/js/safe_outputs_handlers.test.cjs @@ -585,6 +585,30 @@ describe("safe_outputs_handlers", () => { delete process.env.GITHUB_REF_NAME; } }); + + it("should fail closed when patch_format resolves to an invalid value", async () => { + handlers = createHandlers(mockServer, mockAppendSafeOutput, { + create_pull_request: { + patch_format: "invalid-format", + }, + }); + + const result = await handlers.createPullRequestHandler({ + branch: "feature-branch", + title: "Test PR", + body: "Test description", + }); + + expect(result.isError).toBe(true); + const responseData = JSON.parse(result.content[0].text); + expect(responseData.result).toBe("error"); + expect(responseData.error).toContain("Invalid patch_format"); + expect(responseData.error).toContain("invalid-format"); + expect(responseData.error).toContain("am"); + expect(responseData.error).toContain("bundle"); + // Must not have appended any safe output + expect(mockAppendSafeOutput).not.toHaveBeenCalled(); + }); }); describe("pushToPullRequestBranchHandler", () => { @@ -778,6 +802,28 @@ describe("safe_outputs_handlers", () => { delete process.env.GITHUB_BASE_REF; } }); + + it("should fail closed when patch_format resolves to an invalid value", async () => { + handlers = createHandlers(mockServer, mockAppendSafeOutput, { + push_to_pull_request_branch: { + patch_format: "invalid-format", + }, + }); + + const result = await handlers.pushToPullRequestBranchHandler({ + branch: "feature-branch", + }); + + expect(result.isError).toBe(true); + const responseData = JSON.parse(result.content[0].text); + expect(responseData.result).toBe("error"); + expect(responseData.error).toContain("Invalid patch_format"); + expect(responseData.error).toContain("invalid-format"); + expect(responseData.error).toContain("am"); + expect(responseData.error).toContain("bundle"); + // Must not have appended any safe output + expect(mockAppendSafeOutput).not.toHaveBeenCalled(); + }); }); describe("handler structure", () => { diff --git a/docs/src/content/docs/reference/safe-outputs-pull-requests.md b/docs/src/content/docs/reference/safe-outputs-pull-requests.md index 263e6affd8a..a828926fd39 100644 --- a/docs/src/content/docs/reference/safe-outputs-pull-requests.md +++ b/docs/src/content/docs/reference/safe-outputs-pull-requests.md @@ -402,6 +402,50 @@ safe-outputs: When protected file protection triggers and is set to `blocked`, the 🛡️ **Protected Files** section appears in the agent failure issue or comment generated by the conclusion job. It includes the blocked operation, the specific files found, and a YAML remediation snippet showing how to configure `protected-files: fallback-to-issue`. +### Parameterising Policy Fields in Reusable Workflows + +Both `protected-files` and `patch-format` accept **GitHub Actions expression strings** so that reusable `workflow_call` workflows can let callers choose the policy without duplicating the workflow file. + +```yaml wrap +on: + workflow_call: + inputs: + protected-files-policy: + type: string + default: fallback-to-issue + description: > + Protected-file policy: 'blocked', 'fallback-to-issue', or 'allowed'. + patch-format: + type: string + default: am + description: Transport format: 'am' (default) or 'bundle'. +--- +safe-outputs: + push-to-pull-request-branch: + protected-files: ${{ inputs.protected-files-policy }} + patch-format: ${{ inputs.patch-format }} + + create-pull-request: + protected-files: ${{ inputs.protected-files-policy }} + patch-format: ${{ inputs.patch-format }} +``` + +**Literal values are still validated at compile time.** Expression strings are passed through to the runtime config where they are evaluated by GitHub Actions before the handler runs. If the resolved value is not one of the documented allowed values, the handler fails closed: + +- `protected-files`: an unrecognised resolved value is treated as `blocked` (deny — most restrictive). +- `patch-format`: an unrecognised resolved value results in an explicit error before any git operations. + +The object form of `protected-files` also accepts an expression for `policy`: + +```yaml wrap +safe-outputs: + create-pull-request: + protected-files: + policy: ${{ inputs.protected-files-policy }} + exclude: + - AGENTS.md # always exclude — regardless of policy +``` + ### Restricting Changes to Specific Files with `allowed-files` Use `allowed-files` to restrict a safe output to a fixed set of files. When set, it acts as an **exclusive allowlist**: every file touched by the patch must match at least one pattern, and any file outside the list is always refused — including normal source files. The `allowed-files` and `protected-files` checks are **orthogonal**: both run independently and both must pass. To modify a protected file, it must both match `allowed-files` **and** `protected-files` must be set to `allowed`. diff --git a/pkg/parser/schema_test.go b/pkg/parser/schema_test.go index b4397a0fbfb..31497d008e3 100644 --- a/pkg/parser/schema_test.go +++ b/pkg/parser/schema_test.go @@ -878,6 +878,54 @@ func TestValidateMainWorkflowFrontmatterWithSchemaAndLocation_ProtectedFilesObje }, wantErr: false, }, + { + name: "create-pull-request: expression string for protected-files passes", + safeOutputs: map[string]any{ + "create-pull-request": map[string]any{ + "protected-files": "${{ inputs.protected-files-policy }}", + }, + }, + wantErr: false, + }, + { + name: "create-pull-request: expression string for patch-format passes", + safeOutputs: map[string]any{ + "create-pull-request": map[string]any{ + "patch-format": "${{ inputs.patch-format }}", + }, + }, + wantErr: false, + }, + { + name: "push-to-pull-request-branch: expression string for protected-files passes", + safeOutputs: map[string]any{ + "push-to-pull-request-branch": map[string]any{ + "protected-files": "${{ inputs.protected-files-policy }}", + }, + }, + wantErr: false, + }, + { + name: "push-to-pull-request-branch: expression string for patch-format passes", + safeOutputs: map[string]any{ + "push-to-pull-request-branch": map[string]any{ + "patch-format": "${{ inputs.patch-format }}", + }, + }, + wantErr: false, + }, + { + name: "create-pull-request: object form with expression policy passes", + safeOutputs: map[string]any{ + "create-pull-request": map[string]any{ + "protected-files": map[string]any{ + "policy": "${{ inputs.policy }}", + "exclude": []any{"AGENTS.md"}, + }, + }, + }, + wantErr: false, + }, } for _, tt := range tests { diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index aa0eb4db4ae..5e952ff7ba3 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -5988,14 +5988,28 @@ "description": "Controls protected-file protection. blocked (default): hard-block any patch that modifies package manifests (e.g. package.json, go.mod), engine instruction files (e.g. AGENTS.md, CLAUDE.md) or .github/ files. allowed: allow all changes. fallback-to-issue: push the branch but create a review issue instead of a PR, so a human can review the manifest changes before merging.", "default": "blocked" }, + { + "type": "string", + "pattern": "^\\$\\{\\{.*\\}\\}$", + "description": "GitHub Actions expression that resolves to 'blocked', 'allowed', or 'fallback-to-issue' at runtime. Use in reusable workflow_call workflows to parameterise the policy per caller." + }, { "type": "object", "properties": { "policy": { - "type": "string", - "enum": ["blocked", "allowed", "fallback-to-issue"], - "description": "Protection policy. blocked (default): hard-block any patch that modifies protected files. allowed: allow all changes. fallback-to-issue: push the branch but create a review issue instead of a PR.", - "default": "blocked" + "oneOf": [ + { + "type": "string", + "enum": ["blocked", "allowed", "fallback-to-issue"], + "description": "Protection policy. blocked (default): hard-block any patch that modifies protected files. allowed: allow all changes. fallback-to-issue: push the branch but create a review issue instead of a PR.", + "default": "blocked" + }, + { + "type": "string", + "pattern": "^\\$\\{\\{.*\\}\\}$", + "description": "GitHub Actions expression that resolves to 'blocked', 'allowed', or 'fallback-to-issue' at runtime." + } + ] }, "exclude": { "type": "array", @@ -6010,7 +6024,7 @@ "description": "Object form for granular control over the protected-file set. Use the exclude list to remove specific files from the default protection while keeping the rest." } ], - "description": "Controls protected-file protection. String form: blocked (default), allowed, or fallback-to-issue. Object form: { policy, exclude } to customise the protected-file set." + "description": "Controls protected-file protection. String form: blocked (default), allowed, or fallback-to-issue — or a GitHub Actions expression for reusable workflows. Object form: { policy, exclude } to customise the protected-file set." }, "allowed-files": { "type": "array", @@ -6037,10 +6051,20 @@ "description": "List of glob patterns for files to exclude from the patch. Each pattern is passed to `git format-patch` as a `:(exclude)` magic pathspec, so matching files are stripped by git at generation time and will not appear in the commit. Excluded files are also not subject to `allowed-files` or `protected-files` checks. Supports * (any characters except /) and ** (any characters including /)." }, "patch-format": { - "type": "string", - "enum": ["am", "bundle"], - "default": "am", - "description": "Transport format for packaging changes. \"am\" (default) uses git format-patch/git am. \"bundle\" uses git bundle, which preserves merge commit topology, per-commit authorship, and merge-resolution-only content." + "oneOf": [ + { + "type": "string", + "enum": ["am", "bundle"], + "default": "am", + "description": "Transport format for packaging changes. \"am\" (default) uses git format-patch/git am. \"bundle\" uses git bundle, which preserves merge commit topology, per-commit authorship, and merge-resolution-only content." + }, + { + "type": "string", + "pattern": "^\\$\\{\\{.*\\}\\}$", + "description": "GitHub Actions expression that resolves to 'am' or 'bundle' at runtime. Use in reusable workflow_call workflows to parameterise the transport format per caller." + } + ], + "description": "Transport format for packaging changes. \"am\" (default) uses git format-patch/git am. \"bundle\" uses git bundle. Accepts a GitHub Actions expression for reusable workflows." }, "staged": { "type": "boolean", @@ -7305,14 +7329,28 @@ "description": "Controls protected-file protection. blocked (default): hard-block any patch that modifies package manifests (e.g. package.json, go.mod), engine instruction files (e.g. AGENTS.md, CLAUDE.md) or .github/ files. allowed: allow all changes. fallback-to-issue: create a review issue instead of pushing to the PR branch, so a human can review the changes before applying.", "default": "blocked" }, + { + "type": "string", + "pattern": "^\\$\\{\\{.*\\}\\}$", + "description": "GitHub Actions expression that resolves to 'blocked', 'allowed', or 'fallback-to-issue' at runtime. Use in reusable workflow_call workflows to parameterise the policy per caller." + }, { "type": "object", "properties": { "policy": { - "type": "string", - "enum": ["blocked", "allowed", "fallback-to-issue"], - "description": "Protection policy. blocked (default): hard-block any patch that modifies protected files. allowed: allow all changes. fallback-to-issue: create a review issue instead of pushing.", - "default": "blocked" + "oneOf": [ + { + "type": "string", + "enum": ["blocked", "allowed", "fallback-to-issue"], + "description": "Protection policy. blocked (default): hard-block any patch that modifies protected files. allowed: allow all changes. fallback-to-issue: create a review issue instead of pushing.", + "default": "blocked" + }, + { + "type": "string", + "pattern": "^\\$\\{\\{.*\\}\\}$", + "description": "GitHub Actions expression that resolves to 'blocked', 'allowed', or 'fallback-to-issue' at runtime." + } + ] }, "exclude": { "type": "array", @@ -7327,7 +7365,7 @@ "description": "Object form for granular control over the protected-file set. Use the exclude list to remove specific files from the default protection while keeping the rest." } ], - "description": "Controls protected-file protection. String form: blocked (default), allowed, or fallback-to-issue. Object form: { policy, exclude } to customise the protected-file set." + "description": "Controls protected-file protection. String form: blocked (default), allowed, or fallback-to-issue — or a GitHub Actions expression for reusable workflows. Object form: { policy, exclude } to customise the protected-file set." }, "allowed-files": { "type": "array", @@ -7344,10 +7382,20 @@ "description": "List of glob patterns for files to exclude from the patch. Each pattern is passed to `git format-patch` as a `:(exclude)` magic pathspec, so matching files are stripped by git at generation time and will not appear in the commit. Excluded files are also not subject to `allowed-files` or `protected-files` checks. Supports * (any characters except /) and ** (any characters including /)." }, "patch-format": { - "type": "string", - "enum": ["am", "bundle"], - "default": "am", - "description": "Transport format for packaging changes. \"am\" (default) uses git format-patch/git am. \"bundle\" uses git bundle, which preserves merge commit topology, per-commit authorship, and merge-resolution-only content." + "oneOf": [ + { + "type": "string", + "enum": ["am", "bundle"], + "default": "am", + "description": "Transport format for packaging changes. \"am\" (default) uses git format-patch/git am. \"bundle\" uses git bundle, which preserves merge commit topology, per-commit authorship, and merge-resolution-only content." + }, + { + "type": "string", + "pattern": "^\\$\\{\\{.*\\}\\}$", + "description": "GitHub Actions expression that resolves to 'am' or 'bundle' at runtime. Use in reusable workflow_call workflows to parameterise the transport format per caller." + } + ], + "description": "Transport format for packaging changes. \"am\" (default) uses git format-patch/git am. \"bundle\" uses git bundle. Accepts a GitHub Actions expression for reusable workflows." }, "allow-workflows": { "type": "boolean", diff --git a/pkg/workflow/compiler_safe_outputs_config_test.go b/pkg/workflow/compiler_safe_outputs_config_test.go index 708d7b86d24..e2f9261b7e5 100644 --- a/pkg/workflow/compiler_safe_outputs_config_test.go +++ b/pkg/workflow/compiler_safe_outputs_config_test.go @@ -2689,3 +2689,91 @@ func TestNoProtectedDotFolderExcludesWhenNoneDotFolderExcluded(t *testing.T) { _, exists := prConfig["protected_dot_folder_excludes"] assert.False(t, exists, "protected_dot_folder_excludes should be absent when no dot-folders excluded") } + +// TestPRPolicyFieldsExpressionsPassThrough verifies that GitHub Actions expression strings +// set on protected-files and patch-format are emitted verbatim into the handler config. +// This enables reusable workflow_call workflows to parameterise these policy fields per caller. +func TestPRPolicyFieldsExpressionsPassThrough(t *testing.T) { + t.Parallel() + + protectedFilesExpr := "${{ inputs.protected-files-policy }}" + patchFormatExpr := "${{ inputs.patch-format }}" + + tests := []struct { + name string + safeOutputs *SafeOutputsConfig + handlerKey string + wantProtected string + wantFormat string + }{ + { + name: "create-pull-request: expression values pass through", + safeOutputs: &SafeOutputsConfig{ + CreatePullRequests: &CreatePullRequestsConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("1")}, + ManifestFilesPolicy: &protectedFilesExpr, + PatchFormat: patchFormatExpr, + }, + }, + handlerKey: "create_pull_request", + wantProtected: protectedFilesExpr, + wantFormat: patchFormatExpr, + }, + { + name: "push-to-pull-request-branch: expression values pass through", + safeOutputs: &SafeOutputsConfig{ + PushToPullRequestBranch: &PushToPullRequestBranchConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("1")}, + ManifestFilesPolicy: &protectedFilesExpr, + PatchFormat: patchFormatExpr, + }, + }, + handlerKey: "push_to_pull_request_branch", + wantProtected: protectedFilesExpr, + wantFormat: patchFormatExpr, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + compiler := NewCompiler() + workflowData := &WorkflowData{ + Name: "Test Workflow", + SafeOutputs: tt.safeOutputs, + } + + var steps []string + compiler.addHandlerManagerConfigEnvVar(&steps, workflowData) + require.NotEmpty(t, steps, "should produce config steps") + + // Extract handler-config JSON + var configJSON string + for _, step := range steps { + if strings.Contains(step, "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG") { + parts := strings.Split(step, "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG: ") + require.Len(t, parts, 2, "should split env var line") + configJSON = strings.TrimSpace(parts[1]) + configJSON = strings.Trim(configJSON, "\"") + configJSON = strings.ReplaceAll(configJSON, "\\\"", "\"") + } + } + require.NotEmpty(t, configJSON, "should have extracted JSON") + + var config map[string]map[string]any + require.NoError(t, json.Unmarshal([]byte(configJSON), &config), "config JSON should be valid") + + handlerConfig, ok := config[tt.handlerKey] + require.True(t, ok, "should have %s config", tt.handlerKey) + + // protected_files_policy must contain the expression verbatim + pfPolicy, ok := handlerConfig["protected_files_policy"] + require.True(t, ok, "should have protected_files_policy field") + assert.Equal(t, tt.wantProtected, pfPolicy, "protected_files_policy should contain the expression") + + // patch_format must contain the expression verbatim + patchFmt, ok := handlerConfig["patch_format"] + require.True(t, ok, "should have patch_format field") + assert.Equal(t, tt.wantFormat, patchFmt, "patch_format should contain the expression") + }) + } +} diff --git a/pkg/workflow/validation_helpers.go b/pkg/workflow/validation_helpers.go index 86289e94a13..7c041cac215 100644 --- a/pkg/workflow/validation_helpers.go +++ b/pkg/workflow/validation_helpers.go @@ -105,6 +105,10 @@ func validateMountStringFormat(mount string) (source, dest, mode string, err err // of the allowed string values. Non-string values and unrecognised strings are // removed from the map (treated as absent) and a warning is logged. Use this // for fields that are pure string enums with no boolean shorthand. +// +// GitHub Actions expression strings (e.g. "${{ inputs.policy }}") are accepted +// without enum validation and passed through unchanged; the resolved value is +// validated at runtime by the safe-output handler. func validateStringEnumField(configData map[string]any, fieldName string, allowed []string, log *logger.Logger) { if configData == nil { return @@ -114,7 +118,21 @@ func validateStringEnumField(configData map[string]any, fieldName string, allowe return } strVal, ok := val.(string) - if !ok || !slices.Contains(allowed, strVal) { + if !ok { + if log != nil { + log.Printf("Invalid %s value %v (must be one of %v), ignoring", fieldName, val, allowed) + } + delete(configData, fieldName) + return + } + // GitHub Actions expressions are validated at runtime by the handler. + if containsExpression(strVal) { + if log != nil { + log.Printf("%s value is a GitHub Actions expression, skipping compile-time enum validation", fieldName) + } + return + } + if !slices.Contains(allowed, strVal) { if log != nil { log.Printf("Invalid %s value %v (must be one of %v), ignoring", fieldName, val, allowed) } diff --git a/pkg/workflow/validation_helpers_test.go b/pkg/workflow/validation_helpers_test.go index 3c653137f6c..3dfa19a5ed1 100644 --- a/pkg/workflow/validation_helpers_test.go +++ b/pkg/workflow/validation_helpers_test.go @@ -644,3 +644,124 @@ func TestPreprocessProtectedFilesField(t *testing.T) { }) } } + +// TestValidateStringEnumField tests the validateStringEnumField helper. +func TestValidateStringEnumField(t *testing.T) { + allowed := []string{"am", "bundle"} + tests := []struct { + name string + configData map[string]any + wantPresent bool + wantValue any + }{ + { + name: "valid enum value is kept", + configData: map[string]any{"patch-format": "am"}, + wantPresent: true, + wantValue: "am", + }, + { + name: "another valid enum value is kept", + configData: map[string]any{"patch-format": "bundle"}, + wantPresent: true, + wantValue: "bundle", + }, + { + name: "invalid literal string is removed", + configData: map[string]any{"patch-format": "invalid"}, + wantPresent: false, + }, + { + name: "non-string value is removed", + configData: map[string]any{"patch-format": 42}, + wantPresent: false, + }, + { + name: "absent field is a no-op", + configData: map[string]any{"other": "value"}, + wantPresent: false, + }, + { + name: "nil configData is a no-op", + configData: nil, + wantPresent: false, + }, + { + name: "GitHub Actions expression is passed through", + configData: map[string]any{"patch-format": "${{ inputs.patch-format }}"}, + wantPresent: true, + wantValue: "${{ inputs.patch-format }}", + }, + { + name: "expression with spaces is passed through", + configData: map[string]any{"patch-format": "${{ inputs.format || 'am' }}"}, + wantPresent: true, + wantValue: "${{ inputs.format || 'am' }}", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + validateStringEnumField(tt.configData, "patch-format", allowed, nil) + if tt.configData == nil { + return + } + val, present := tt.configData["patch-format"] + assert.Equal(t, tt.wantPresent, present, "field presence should match") + if tt.wantPresent { + assert.Equal(t, tt.wantValue, val, "field value should match") + } + }) + } +} + +// TestPreprocessProtectedFilesFieldWithExpression tests that GitHub Actions expressions +// in the object-form policy field pass through the preprocessing step unchanged. +func TestPreprocessProtectedFilesFieldWithExpression(t *testing.T) { + tests := []struct { + name string + configData map[string]any + wantExclude []string + wantPFAfter any + wantPFPresent bool + }{ + { + name: "expression string form passes through unchanged", + configData: map[string]any{ + "protected-files": "${{ inputs.protected-files-policy }}", + }, + wantExclude: nil, + wantPFAfter: "${{ inputs.protected-files-policy }}", + wantPFPresent: true, + }, + { + name: "object form with expression policy", + configData: map[string]any{ + "protected-files": map[string]any{ + "policy": "${{ inputs.policy }}", + "exclude": []any{"AGENTS.md"}, + }, + }, + wantExclude: []string{"AGENTS.md"}, + wantPFAfter: "${{ inputs.policy }}", + wantPFPresent: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := preprocessProtectedFilesField(tt.configData, nil) + if len(tt.wantExclude) == 0 { + assert.Empty(t, got, "exclude list should be empty/nil") + } else { + assert.Equal(t, tt.wantExclude, got, "exclude list should match") + } + + pfVal, pfPresent := tt.configData["protected-files"] + assert.Equal(t, tt.wantPFPresent, pfPresent, "protected-files presence should match") + if tt.wantPFPresent { + assert.Equal(t, tt.wantPFAfter, pfVal, "protected-files value should match after preprocessing") + } + }) + } +} From 81fc48f1019720a159c66fc43b92e1b8d40e6c0d Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 30 Apr 2026 01:55:30 +0000 Subject: [PATCH 3/4] docs(adr): add draft ADR-29230 for parameterized safe-output policy fields Records the architectural decision to accept GitHub Actions expression strings for protected-files and patch-format fields, enabling reusable workflow_call workflows to parameterize policy per caller. Co-Authored-By: Claude Sonnet 4.6 --- ...-output-policy-fields-for-workflow-call.md | 77 +++++++++++++++++++ 1 file changed, 77 insertions(+) create mode 100644 docs/adr/29230-parameterize-safe-output-policy-fields-for-workflow-call.md diff --git a/docs/adr/29230-parameterize-safe-output-policy-fields-for-workflow-call.md b/docs/adr/29230-parameterize-safe-output-policy-fields-for-workflow-call.md new file mode 100644 index 00000000000..5d14f785a79 --- /dev/null +++ b/docs/adr/29230-parameterize-safe-output-policy-fields-for-workflow-call.md @@ -0,0 +1,77 @@ +# ADR-29230: Parameterize Safe-Output Policy Fields for `workflow_call` Reuse + +**Date**: 2026-04-30 +**Status**: Draft +**Deciders**: pelikhan + +--- + +## Part 1 — Narrative (Human-Friendly) + +### Context + +The `safe-outputs` configuration block for `create-pull-request` and `push-to-pull-request-branch` exposes two behavior-controlling fields: `protected-files` (policy: `blocked`, `allowed`, or `fallback-to-issue`) and `patch-format` (`am` or `bundle`). Both fields were previously restricted to compile-time literal enum values. This meant that reusable `workflow_call` workflows could not expose these as caller-controlled inputs — any team needing a different policy had to either fork the workflow file or maintain one copy per policy combination, creating duplicated YAML that diverged over time. The prior work in PR #29212 established the pattern of accepting GitHub Actions expression strings for list-valued constraints; this PR extends that pattern to enum-valued policy fields. + +### Decision + +We will allow `protected-files` (both string form and the `policy` key of its object form) and `patch-format` to accept GitHub Actions expression strings (matching `${{...}}`) in addition to their existing literal enum values. Literal values continue to be validated at compile time and rejected if unrecognized. Expression strings are passed through the compiler unchanged and emitted verbatim into the runtime handler configuration, where GitHub Actions evaluates them before the handler executes. The runtime handlers validate the resolved value and fail closed — `patch-format` returns an explicit error for unrecognized values; `protected-files` treats unrecognized values as `blocked` (most restrictive default). + +### Alternatives Considered + +#### Alternative 1: Duplicate Workflow Files per Policy Variant + +Callers could maintain separate copies of the reusable workflow for each combination of `protected-files` and `patch-format` values. This was the status quo before this PR. It was rejected because it creates maintainability debt: every structural change to the base workflow must be propagated to all variants, and teams routinely drift their copies. The pattern does not scale as the number of callers grows. + +#### Alternative 2: Accept Free-Form Strings with No Compile-Time Detection + +The schema could be relaxed to accept any string (dropping enum enforcement entirely) and leave all validation to the runtime handler. This was not chosen because it would silently pass obviously invalid literal values (e.g., typos like `"blocekd"`) through compilation and only fail at job execution time, degrading the developer experience and breaking the "fail fast at compile time" guarantee that the rest of the system provides. Detecting expression strings by the `${{...}}` pattern preserves compile-time strictness for literals while enabling dynamic resolution for expressions. + +#### Alternative 3: Introduce a New Dedicated `policy-expression` Field + +A new field (e.g., `protected-files-expression`) could accept only expressions while the original field remains enum-only. This avoids changing the type of existing fields but doubles the surface area and requires callers to know which field to use in which context. It was rejected as unnecessarily complex when the pattern of "enum literal OR expression string" is both clear and consistent with how other expression-accepting fields work in GitHub Actions schemas. + +### Consequences + +#### Positive +- A single reusable `workflow_call` workflow can serve callers with different `protected-files` policies and `patch-format` requirements without duplicating files. +- Literal enum values retain compile-time validation and early error reporting; nothing regresses for existing non-expression usage. +- The fail-closed runtime behavior (`blocked` for unknown policy, explicit error for unknown patch format) ensures that expression misconfiguration cannot silently weaken security posture. +- The pattern is consistent with the expression-acceptance approach introduced in PR #29212 for list-valued constraints. + +#### Negative +- Expression values are only validated at runtime, after GitHub Actions evaluates them. A typo in an input default (e.g., `default: blocekd`) will not be caught until the workflow runs. +- Two-stage validation logic (compile-time for literals, runtime for expressions) adds complexity to both the Go `validateStringEnumField` helper and the JavaScript handlers. +- The JSON schema now uses `oneOf` for fields that were previously a simple `enum`, which may complicate tooling (e.g., IDE autocomplete may not suggest enum values when the field contains an expression). + +#### Neutral +- The `containsExpression` helper used to detect `${{...}}` patterns was already available in the codebase; this PR reuses it rather than introducing a new detection mechanism. +- Documentation in `docs/src/content/docs/reference/safe-outputs-pull-requests.md` was updated to show the `workflow_call` usage pattern with explicit examples. +- Schema changes apply symmetrically to both `create-pull-request` and `push-to-pull-request-branch` handler blocks. + +--- + +## 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). + +### Schema and Compile-Time Validation + +1. The `protected-files` field and the `policy` sub-key of the `protected-files` object form **MUST** accept either a literal enum value (`blocked`, `allowed`, `fallback-to-issue`) or a GitHub Actions expression string matching the pattern `^\$\{\{.*\}\}$`. +2. The `patch-format` field **MUST** accept either a literal enum value (`am`, `bundle`) or a GitHub Actions expression string matching the pattern `^\$\{\{.*\}\}$`. +3. Literal enum values **MUST** be validated at compile time; unrecognized literal values **MUST** be rejected (removed from the config with a warning log) before the workflow is emitted. +4. GitHub Actions expression strings **MUST NOT** be enum-validated at compile time; they **MUST** be passed through to the runtime handler configuration verbatim. +5. The `validateStringEnumField` helper **MUST** use the `containsExpression` function to distinguish expressions from literal strings before applying enum validation. + +### Runtime Handler Validation + +1. Handlers **MUST** validate the resolved value of `patch_format` after GitHub Actions evaluates any expression; if the resolved value is not in `["am", "bundle"]`, the handler **MUST** return an error response and **MUST NOT** perform any git operations. +2. Handlers **MUST** validate the resolved value of `protected_files_policy` after evaluation; if the resolved value is not a recognized policy, the handler **MUST** treat it as `blocked` (fail closed, most restrictive). +3. Neither handler **MUST NOT** append any safe output to the output file when returning an error due to an invalid resolved field value. + +### 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 — in particular: allowing unrecognized literal values past compile time, performing git operations after a resolved-value validation failure, or treating an unrecognized `protected_files_policy` as anything other than `blocked` — constitutes non-conformance. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/25143053856) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* From f3b2fc76d52e7c1659139421dd61fd5c4cd814ad Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 30 Apr 2026 02:09:28 +0000 Subject: [PATCH 4/4] fix: patch_format runtime validation security and correctness improvements Agent-Logs-Url: https://github.com/github/gh-aw/sessions/f35fc6e6-684c-42d5-82d5-3e2ccca5b4b5 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- actions/setup/js/safe_outputs_handlers.cjs | 12 +++-- .../setup/js/safe_outputs_handlers.test.cjs | 44 ++++++++++++++++++- 2 files changed, 50 insertions(+), 6 deletions(-) diff --git a/actions/setup/js/safe_outputs_handlers.cjs b/actions/setup/js/safe_outputs_handlers.cjs index d5d015f72e1..606024574e9 100644 --- a/actions/setup/js/safe_outputs_handlers.cjs +++ b/actions/setup/js/safe_outputs_handlers.cjs @@ -319,10 +319,12 @@ function createHandlers(server, appendSafeOutput, config = {}) { // Determine transport format: "bundle" uses git bundle (preserves merge topology), // "am" (default) uses git format-patch / git am (good for linear histories). - const patchFormat = prConfig["patch_format"] || config["patch_format"] || "am"; + // Use ?? (nullish coalescing) so an empty-string resolved value is preserved and + // rejected below rather than silently falling back to "am". + const patchFormat = prConfig["patch_format"] ?? config["patch_format"] ?? "am"; const validPatchFormats = ["am", "bundle"]; if (!validPatchFormats.includes(patchFormat)) { - const errorMsg = `Invalid patch_format: "${patchFormat}". Must be one of: ${validPatchFormats.join(", ")}`; + const errorMsg = `Invalid patch_format in configuration. Must be one of: ${validPatchFormats.join(", ")}`; server.debug(`create_pull_request: ${errorMsg}`); return { content: [ @@ -547,10 +549,12 @@ function createHandlers(server, appendSafeOutput, config = {}) { // Determine transport format: "bundle" uses git bundle (preserves merge topology), // "am" (default) uses git format-patch / git am (good for linear histories). - const pushPatchFormat = pushConfig["patch_format"] || config["patch_format"] || "am"; + // Use ?? (nullish coalescing) so an empty-string resolved value is preserved and + // rejected below rather than silently falling back to "am". + const pushPatchFormat = pushConfig["patch_format"] ?? config["patch_format"] ?? "am"; const validPushPatchFormats = ["am", "bundle"]; if (!validPushPatchFormats.includes(pushPatchFormat)) { - const errorMsg = `Invalid patch_format: "${pushPatchFormat}". Must be one of: ${validPushPatchFormats.join(", ")}`; + const errorMsg = `Invalid patch_format in configuration. Must be one of: ${validPushPatchFormats.join(", ")}`; server.debug(`push_to_pull_request_branch: ${errorMsg}`); return { content: [ diff --git a/actions/setup/js/safe_outputs_handlers.test.cjs b/actions/setup/js/safe_outputs_handlers.test.cjs index 7d845354fdb..b51a8672530 100644 --- a/actions/setup/js/safe_outputs_handlers.test.cjs +++ b/actions/setup/js/safe_outputs_handlers.test.cjs @@ -603,12 +603,33 @@ describe("safe_outputs_handlers", () => { const responseData = JSON.parse(result.content[0].text); expect(responseData.result).toBe("error"); expect(responseData.error).toContain("Invalid patch_format"); - expect(responseData.error).toContain("invalid-format"); expect(responseData.error).toContain("am"); expect(responseData.error).toContain("bundle"); + // Must not echo the raw resolved value (could be a secret expression result) + expect(responseData.error).not.toContain("invalid-format"); // Must not have appended any safe output expect(mockAppendSafeOutput).not.toHaveBeenCalled(); }); + + it("should fail closed when patch_format resolves to an empty string", async () => { + handlers = createHandlers(mockServer, mockAppendSafeOutput, { + create_pull_request: { + patch_format: "", + }, + }); + + const result = await handlers.createPullRequestHandler({ + branch: "feature-branch", + title: "Test PR", + body: "Test description", + }); + + expect(result.isError).toBe(true); + const responseData = JSON.parse(result.content[0].text); + expect(responseData.result).toBe("error"); + expect(responseData.error).toContain("Invalid patch_format"); + expect(mockAppendSafeOutput).not.toHaveBeenCalled(); + }); }); describe("pushToPullRequestBranchHandler", () => { @@ -818,12 +839,31 @@ describe("safe_outputs_handlers", () => { const responseData = JSON.parse(result.content[0].text); expect(responseData.result).toBe("error"); expect(responseData.error).toContain("Invalid patch_format"); - expect(responseData.error).toContain("invalid-format"); expect(responseData.error).toContain("am"); expect(responseData.error).toContain("bundle"); + // Must not echo the raw resolved value (could be a secret expression result) + expect(responseData.error).not.toContain("invalid-format"); // Must not have appended any safe output expect(mockAppendSafeOutput).not.toHaveBeenCalled(); }); + + it("should fail closed when patch_format resolves to an empty string", async () => { + handlers = createHandlers(mockServer, mockAppendSafeOutput, { + push_to_pull_request_branch: { + patch_format: "", + }, + }); + + const result = await handlers.pushToPullRequestBranchHandler({ + branch: "feature-branch", + }); + + expect(result.isError).toBe(true); + const responseData = JSON.parse(result.content[0].text); + expect(responseData.result).toBe("error"); + expect(responseData.error).toContain("Invalid patch_format"); + expect(mockAppendSafeOutput).not.toHaveBeenCalled(); + }); }); describe("handler structure", () => {