Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions actions/setup/js/manifest_file_helpers.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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");
});
});
});
42 changes: 40 additions & 2 deletions actions/setup/js/safe_outputs_handlers.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,26 @@ 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 in configuration. 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
Expand Down Expand Up @@ -530,7 +549,26 @@ 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 in configuration. 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
Expand Down
86 changes: 86 additions & 0 deletions actions/setup/js/safe_outputs_handlers.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -585,6 +585,51 @@ 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("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", () => {
Expand Down Expand Up @@ -778,6 +823,47 @@ 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("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", () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -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.*
44 changes: 44 additions & 0 deletions docs/src/content/docs/reference/safe-outputs-pull-requests.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down
48 changes: 48 additions & 0 deletions pkg/parser/schema_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading