diff --git a/docs/adr/26339-protected-files-exclude-option.md b/docs/adr/26339-protected-files-exclude-option.md new file mode 100644 index 00000000000..dcc1502d6e4 --- /dev/null +++ b/docs/adr/26339-protected-files-exclude-option.md @@ -0,0 +1,89 @@ +# ADR-26339: Extend `protected-files` to Support Exclusion Overrides + +**Date**: 2026-04-15 +**Status**: Draft +**Deciders**: pelikhan, Copilot + +--- + +## Part 1 — Narrative (Human-Friendly) + +### Context + +The `protected-files` configuration field on `create-pull-request` and `push-to-pull-request-branch` handlers controls whether patches that touch protected files (package manifests, engine instruction files such as `AGENTS.md` / `CLAUDE.md`, and `.github/` paths) are blocked, allowed, or redirected to a review issue. The list of protected files is compiled from a hardcoded manifest and cannot be narrowed per workflow. Workflows that legitimately need to update `AGENTS.md` or similar instruction files have no escape hatch: setting `protected-files: allowed` disables all protection (too permissive), while `blocked` and `fallback-to-issue` still reject the targeted file alongside every other protected file. + +### Decision + +We will extend the `protected-files` field to accept either the existing string enum (`"blocked"`, `"allowed"`, `"fallback-to-issue"`) or a new object form `{ policy: string, exclude: []string }`. The `exclude` list names files or path prefixes that should be removed from the default protected set at compile time, leaving the rest of the protection intact. The object form is normalized in-place during parsing via `preprocessProtectedFilesField` so the rest of the compiler pipeline sees only the (now-narrowed) protected file list — no runtime changes are required. + +### Alternatives Considered + +#### Alternative 1: Global `allowed` policy + +Setting `protected-files: allowed` already allows agents to touch any file. It was rejected because it is all-or-nothing: a workflow that only needs to modify `AGENTS.md` would also lose protection for `go.mod`, `package.json`, and `.github/` files, undermining the purpose of the guard entirely. + +#### Alternative 2: Separate top-level `protected-files-exclude` field + +Adding a sibling YAML key (e.g., `protected-files-exclude: [AGENTS.md]`) would have been schema-additive without touching `protected-files`. It was rejected in favour of the object form because grouping `policy` and `exclude` under a single key makes the relationship explicit: the two sub-fields configure the same gate and should be read together by workflow authors. + +#### Alternative 3: `allowed-files` allowlist + +The existing `allowed-files` field is a strict *allowlist* of patterns that must pass for any file the agent touches. Its semantics are orthogonal to `protected-files`' *blocklist* semantics, and the two checks are evaluated independently. Using `allowed-files` to carve out exceptions from the protected set is unintuitive and would not actually suppress the protected-files check — it would only add a second, unrelated gate. + +#### Alternative 4: Compile-time generated separate configuration key + +Emitting a new runtime key (e.g., `protected_files_exclude`) alongside `protected_files` and teaching the runtime handler to apply the exclusion at execution time was considered. It was rejected because it would require coordinated changes to every runtime handler that reads `protected_files`, expanding the blast radius significantly. The compile-time sentinel approach (`_protected_files_exclude`) confines all filtering logic to the compiler and requires zero runtime changes. + +### Consequences + +#### Positive +- Workflow authors can unblock specific AI-modifiable instruction files (e.g., `AGENTS.md`) without degrading protection for dependency manifests or CI configuration. +- The string form is fully backward-compatible — no existing workflow configuration changes are required. +- Import set-merge semantics ensure that a base workflow imported via `imports:` can contribute exclusions without overwriting the importing workflow's full handler configuration. +- The sentinel key (`_protected_files_exclude`) is stripped before the runtime `config.json` is emitted, so the runtime handler API surface is unchanged. + +#### Negative +- `protected-files` is now a polymorphic `oneOf` field (string | object). JSON Schema validators and tooling that previously relied on it being a simple string enum require schema updates. +- `preprocessProtectedFilesField` mutates `configData` in-place as a side-effect, which is non-obvious for future maintainers reading the parsing code path. +- The `ProtectedFilesExclude []string` field on `CreatePullRequestsConfig` and `PushToPullRequestBranchConfig` uses `yaml:"-"` to prevent direct YAML unmarshaling, relying on the pre-processing step. Forgetting this contract during refactors could silently result in exclusions being ignored. + +#### Neutral +- The `excludeFromSlice` and `mergeUnique` utilities added in `runtime_definitions.go` are general-purpose helpers that may prove reusable for future protected-path filtering work. +- All exclusion logic is concentrated in the compiler layer; the runtime handler receives a pre-filtered `protected_files` list and is unaware of the `exclude` feature. +- Tests cover the sentinel stripping, the import-merge set semantics, and the preprocessing helper, providing regression coverage for the non-obvious two-phase design. + +--- + +## 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). + +### Configuration Schema + +1. The `protected-files` field **MUST** accept either a plain string enum value (`"blocked"`, `"allowed"`, or `"fallback-to-issue"`) or an object with an optional `policy` string and an optional `exclude` string array. +2. When the object form is used, the `policy` sub-field **MUST** be one of the same three enum values if present; an absent or empty `policy` **MUST** be treated as equivalent to `"blocked"`. +3. The `exclude` sub-field **MUST** be treated as a list of filenames or path prefixes to remove from the default protected file set; each entry **MUST** be matched by basename (e.g., `"AGENTS.md"`) or path prefix (e.g., `".agents/"`). +4. Additional properties on the object form **MUST NOT** be accepted; the JSON Schema `additionalProperties: false` constraint **MUST** be enforced. + +### Compile-Time Preprocessing + +1. The compiler **MUST** invoke `preprocessProtectedFilesField` before YAML unmarshaling of the handler config struct so that the object form is normalized to a plain policy string. +2. When the object form is encountered, the compiler **MUST** replace the `"protected-files"` key with the extracted policy string (or delete it when no policy is specified) so that downstream `validateStringEnumField` calls see only a string or an absent key. +3. The compiler **MUST** propagate the extracted exclude list to the handler config struct via `ProtectedFilesExclude`; it **MUST NOT** forward the exclusions as part of the serialized YAML. +4. The compiler **MUST** emit the sentinel key `_protected_files_exclude` in the handler registry config map to carry exclusions from the handler builder to `addHandlerManagerConfigEnvVar`. +5. `addHandlerManagerConfigEnvVar` **MUST** read and delete the `_protected_files_exclude` sentinel before serializing the runtime config so that the sentinel **MUST NOT** appear in the environment variable or in `config.json`. +6. `generateSafeOutputsConfig` **MUST** also delete `_protected_files_exclude` from any handler config before writing `config.json`. + +### Import Merge Semantics + +1. When `MergeSafeOutputs` processes an imported config whose handler type conflicts with the top-level config, it **MUST** extract and accumulate the `protected-files.exclude` list from the imported config before discarding that handler entry. +2. Accumulated exclusion lists **MUST** be merged as a deduplicated set into the top-level handler config's `ProtectedFilesExclude` field after all imports are processed. +3. `mergeSafeOutputConfig` **MUST** also merge `ProtectedFilesExclude` as a deduplicated set when both the top-level and imported config define the same handler type. + +### 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/24431500261) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index ec59a0ce135..86adfc2efbe 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -5767,10 +5767,34 @@ "description": "Token used to push an empty commit after PR creation to trigger CI events. Works around the GITHUB_TOKEN limitation where pushes don't trigger workflow runs. Defaults to the magic secret GH_AW_CI_TRIGGER_TOKEN if set in the repository. Use a secret expression (e.g. '${{ secrets.CI_TOKEN }}') for a custom token, or 'app' for GitHub App auth." }, "protected-files": { - "type": "string", - "enum": ["blocked", "allowed", "fallback-to-issue"], - "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" + "oneOf": [ + { + "type": "string", + "enum": ["blocked", "allowed", "fallback-to-issue"], + "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": "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" + }, + "exclude": { + "type": "array", + "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/"]] + } + }, + "additionalProperties": false, + "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." }, "allowed-files": { "type": "array", @@ -6937,10 +6961,34 @@ "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." }, "protected-files": { - "type": "string", - "enum": ["blocked", "allowed", "fallback-to-issue"], - "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" + "oneOf": [ + { + "type": "string", + "enum": ["blocked", "allowed", "fallback-to-issue"], + "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": "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" + }, + "exclude": { + "type": "array", + "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/"]] + } + }, + "additionalProperties": false, + "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." }, "allowed-files": { "type": "array", diff --git a/pkg/workflow/compiler_safe_outputs_config.go b/pkg/workflow/compiler_safe_outputs_config.go index e99b5c3237f..c6791451737 100644 --- a/pkg/workflow/compiler_safe_outputs_config.go +++ b/pkg/workflow/compiler_safe_outputs_config.go @@ -64,8 +64,13 @@ func (c *Compiler) addHandlerManagerConfigEnvVar(steps *[]string, data *Workflow if handlerConfig != nil { // Augment protected-files protection with engine-specific files for handlers that use it. if _, hasProtected := handlerConfig["protected_files"]; hasProtected { - handlerConfig["protected_files"] = fullManifestFiles - handlerConfig["protected_path_prefixes"] = fullPathPrefixes + // Extract per-handler exclusions set by the handler builder (sentinel key). + // These are compile-time overrides and must not be forwarded to the runtime. + excludeFiles := extractStringSliceFromConfig(handlerConfig, "_protected_files_exclude") + delete(handlerConfig, "_protected_files_exclude") + + handlerConfig["protected_files"] = excludeFromSlice(fullManifestFiles, excludeFiles...) + handlerConfig["protected_path_prefixes"] = excludeFromSlice(fullPathPrefixes, excludeFiles...) } compilerSafeOutputsConfigLog.Printf("Adding %s handler configuration", handlerName) config[handlerName] = handlerConfig diff --git a/pkg/workflow/compiler_safe_outputs_config_test.go b/pkg/workflow/compiler_safe_outputs_config_test.go index 565e3a71f75..6c59b462b6f 100644 --- a/pkg/workflow/compiler_safe_outputs_config_test.go +++ b/pkg/workflow/compiler_safe_outputs_config_test.go @@ -2012,3 +2012,151 @@ func TestAddHandlerManagerConfigEnvVar_CallWorkflow(t *testing.T) { assert.Equal(t, "./.github/workflows/worker-a.lock.yml", filesMap["worker-a"], "worker-a path should match") assert.Equal(t, "./.github/workflows/worker-b.lock.yml", filesMap["worker-b"], "worker-b path should match") } + +// TestProtectedFilesExclude verifies that the _protected_files_exclude sentinel key is +// used at compile time to filter manifest files and is NOT forwarded to the runtime config. +func TestProtectedFilesExclude(t *testing.T) { + tests := []struct { + name string + excludeFiles []string + wantExcludedFromPF []string // files that must NOT be in the final protected_files list + wantPresentInPF []string // files that must still be in the protected_files list + }{ + { + name: "exclude AGENTS.md from create-pull-request", + excludeFiles: []string{"AGENTS.md"}, + wantExcludedFromPF: []string{"AGENTS.md"}, + wantPresentInPF: []string{"package.json", "go.mod", "CODEOWNERS"}, + }, + { + name: "exclude multiple files", + excludeFiles: []string{"AGENTS.md", "CLAUDE.md"}, + wantExcludedFromPF: []string{"AGENTS.md", "CLAUDE.md"}, + wantPresentInPF: []string{"package.json", "go.mod"}, + }, + { + name: "empty exclude list leaves defaults intact", + excludeFiles: nil, + wantExcludedFromPF: nil, + wantPresentInPF: []string{"package.json", "go.mod"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + compiler := NewCompiler() + workflowData := &WorkflowData{ + Name: "Test Workflow", + SafeOutputs: &SafeOutputsConfig{ + CreatePullRequests: &CreatePullRequestsConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("1")}, + ProtectedFilesExclude: tt.excludeFiles, + }, + }, + } + + var steps []string + compiler.addHandlerManagerConfigEnvVar(&steps, workflowData) + require.NotEmpty(t, steps, "should produce config steps") + + stepsContent := strings.Join(steps, "") + require.Contains(t, stepsContent, "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG", "should produce handler config") + + // Extract 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 be able to 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") + + prConfig, ok := config["create_pull_request"] + require.True(t, ok, "should have create_pull_request config") + + // Sentinel key must NOT appear in the final runtime config + _, hasSentinel := prConfig["_protected_files_exclude"] + assert.False(t, hasSentinel, "_protected_files_exclude sentinel must not appear in runtime config") + + // Check protected_files list + pfRaw, ok := prConfig["protected_files"] + require.True(t, ok, "should have protected_files field") + pfAny, ok := pfRaw.([]any) + require.True(t, ok, "protected_files should be a slice") + pfStrings := make([]string, 0, len(pfAny)) + for _, v := range pfAny { + if s, ok := v.(string); ok { + pfStrings = append(pfStrings, s) + } + } + + for _, excluded := range tt.wantExcludedFromPF { + assert.NotContains(t, pfStrings, excluded, + "excluded file %q should not appear in protected_files", excluded) + } + for _, present := range tt.wantPresentInPF { + assert.Contains(t, pfStrings, present, + "non-excluded file %q should still appear in protected_files", present) + } + }) + } +} + +// TestProtectedFilesExcludePushToPRBranch verifies the same exclusion logic for +// the push_to_pull_request_branch handler. +func TestProtectedFilesExcludePushToPRBranch(t *testing.T) { + compiler := NewCompiler() + workflowData := &WorkflowData{ + Name: "Test Workflow", + SafeOutputs: &SafeOutputsConfig{ + PushToPullRequestBranch: &PushToPullRequestBranchConfig{ + ProtectedFilesExclude: []string{"AGENTS.md"}, + }, + }, + } + + var steps []string + compiler.addHandlerManagerConfigEnvVar(&steps, workflowData) + require.NotEmpty(t, steps, "should produce config steps") + + 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") + + pushConfig, ok := config["push_to_pull_request_branch"] + require.True(t, ok, "should have push_to_pull_request_branch config") + + _, hasSentinel := pushConfig["_protected_files_exclude"] + assert.False(t, hasSentinel, "_protected_files_exclude sentinel must not appear in runtime config") + + pfRaw, ok := pushConfig["protected_files"] + require.True(t, ok, "should have protected_files field") + pfAny, ok := pfRaw.([]any) + require.True(t, ok, "protected_files should be a slice") + pfStrings := make([]string, 0, len(pfAny)) + for _, v := range pfAny { + if s, ok := v.(string); ok { + pfStrings = append(pfStrings, s) + } + } + assert.NotContains(t, pfStrings, "AGENTS.md", "AGENTS.md should be excluded from protected_files") + assert.Contains(t, pfStrings, "package.json", "package.json should still be in protected_files") +} diff --git a/pkg/workflow/compiler_safe_outputs_handlers.go b/pkg/workflow/compiler_safe_outputs_handlers.go index 56f9f88fe50..a6ec04e4cb0 100644 --- a/pkg/workflow/compiler_safe_outputs_handlers.go +++ b/pkg/workflow/compiler_safe_outputs_handlers.go @@ -385,6 +385,7 @@ var handlerRegistry = map[string]handlerBuilder{ AddStringPtr("protected_files_policy", c.ManifestFilesPolicy). AddStringSlice("protected_files", getAllManifestFiles()). AddStringSlice("protected_path_prefixes", getProtectedPathPrefixes()). + AddStringSlice("_protected_files_exclude", c.ProtectedFilesExclude). AddStringSlice("allowed_files", c.AllowedFiles). AddStringSlice("excluded_files", c.ExcludedFiles). AddIfTrue("preserve_branch_name", c.PreserveBranchName). @@ -416,6 +417,7 @@ var handlerRegistry = map[string]handlerBuilder{ AddStringPtr("protected_files_policy", c.ManifestFilesPolicy). AddStringSlice("protected_files", getAllManifestFiles()). AddStringSlice("protected_path_prefixes", getProtectedPathPrefixes()). + AddStringSlice("_protected_files_exclude", c.ProtectedFilesExclude). AddStringSlice("allowed_files", c.AllowedFiles). AddStringSlice("excluded_files", c.ExcludedFiles). AddIfNotEmpty("patch_format", c.PatchFormat). diff --git a/pkg/workflow/create_pull_request.go b/pkg/workflow/create_pull_request.go index e9680b2f225..270da2522b4 100644 --- a/pkg/workflow/create_pull_request.go +++ b/pkg/workflow/create_pull_request.go @@ -35,6 +35,7 @@ type CreatePullRequestsConfig struct { AutoCloseIssue *string `yaml:"auto-close-issue,omitempty"` // Auto-add "Fixes #N" closing keyword when triggered from an issue (default: true). Set to false to prevent auto-closing the triggering issue on PR merge. Accepts a boolean or a GitHub Actions expression. GithubTokenForExtraEmptyCommit string `yaml:"github-token-for-extra-empty-commit,omitempty"` // Token used to push an empty commit to trigger CI events. Use a PAT or "app" for GitHub App auth. ManifestFilesPolicy *string `yaml:"protected-files,omitempty"` // Controls protected-file protection: "blocked" (default) hard-blocks, "allowed" permits all changes, "fallback-to-issue" pushes the branch but creates a review issue. + ProtectedFilesExclude []string `yaml:"-"` // Files/prefixes to exclude from the default protected list (from object-form protected-files.exclude). Not sourced from YAML directly; populated during pre-processing. AllowedFiles []string `yaml:"allowed-files,omitempty"` // Strict allowlist of glob patterns for files eligible for create. Checked independently of protected-files; both checks must pass. ExcludedFiles []string `yaml:"excluded-files,omitempty"` // List of glob patterns for files to exclude from the patch using git :(exclude) pathspecs. Matching files are stripped by git at generation time and will not appear in the commit or be subject to allowed-files or protected-files checks. PreserveBranchName bool `yaml:"preserve-branch-name,omitempty"` // When true, skips the random salt suffix on agent-specified branch names. Invalid characters are still replaced for security; casing is always preserved. Useful when CI enforces branch naming conventions (e.g. Jira keys in uppercase). @@ -92,7 +93,15 @@ func (c *Compiler) parsePullRequestsConfig(outputMap map[string]any) *CreatePull } } - // Pre-process protected-files: pure string enum ("blocked", "allowed", "fallback-to-issue"). + // Pre-process protected-files: supports string enum OR object form {policy, exclude}. + // Object form is preprocessed to extract the policy (stored back as string) and + // the exclude list (stored in a local variable and assigned to the config after unmarshaling). + var protectedFilesExclude []string + if configData != nil { + protectedFilesExclude = preprocessProtectedFilesField(configData, createPRLog) + } + + // Validate protected-files string enum after object-form preprocessing. manifestFilesEnums := []string{"blocked", "allowed", "fallback-to-issue"} if configData != nil { validateStringEnumField(configData, "protected-files", manifestFilesEnums, createPRLog) @@ -123,6 +132,9 @@ func (c *Compiler) parsePullRequestsConfig(outputMap map[string]any) *CreatePull createPRLog.Printf("Pull request expiration configured: %d hours", config.Expires) } + // Apply the exclude list extracted from the object-form protected-files field. + config.ProtectedFilesExclude = protectedFilesExclude + // Set default max if not explicitly configured (default is 1) if config.Max == nil { config.Max = defaultIntStr(1) diff --git a/pkg/workflow/imports.go b/pkg/workflow/imports.go index d7b34ab5b42..02344407a91 100644 --- a/pkg/workflow/imports.go +++ b/pkg/workflow/imports.go @@ -219,6 +219,11 @@ func (c *Compiler) MergeSafeOutputs(topSafeOutputs *SafeOutputsConfig, importedS // staged, env, github-token, max-patch-size, runs-on) as well as those defining safe output types. // Meta fields can be imported even when no safe output types are defined. var importedConfigs []map[string]any + // Collect protected-files exclude lists from type-conflicting imports so they can be + // merged as a set even when the importing config already defines the same handler type. + // These are keyed by handler type name (e.g. "create-pull-request"). + accumulatedExclude := map[string][]string{} + for _, configJSON := range importedSafeOutputsJSON { if configJSON == "" || configJSON == "{}" { continue @@ -231,11 +236,22 @@ func (c *Compiler) MergeSafeOutputs(topSafeOutputs *SafeOutputsConfig, importedS } // Check for conflicts and remove types already defined in top-level config - // Main workflow definitions take precedence over imports (override behavior) + // Main workflow definitions take precedence over imports (override behavior). + // Exception: protected-files.exclude is always extracted before deletion so that + // exclude lists from imported configs are merged as a set into the result. for _, key := range typeKeys { if _, exists := config[key]; exists { if topDefinedTypes[key] { - // Main workflow overrides imported definition - remove from imported config + // Main workflow overrides imported definition — extract protected-files + // exclude lists before removing the type entry. + if handlerCfg, ok := config[key].(map[string]any); ok { + if pf, ok := handlerCfg["protected-files"].(map[string]any); ok { + if excludeFiles := parseStringSliceAny(pf["exclude"], importsLog); len(excludeFiles) > 0 { + accumulatedExclude[key] = mergeUnique(accumulatedExclude[key], excludeFiles...) + importsLog.Printf("Saved protected-files exclude from overridden import %s: %v", key, excludeFiles) + } + } + } importsLog.Printf("Main workflow overrides imported safe-output: %s", key) delete(config, key) continue @@ -272,6 +288,30 @@ func (c *Compiler) MergeSafeOutputs(topSafeOutputs *SafeOutputsConfig, importedS } } + // Apply protected-files exclude lists accumulated from type-conflicting imports. + // These are merged as a set so that importing a base workflow can add to exclusions + // without completely replacing the main workflow's handler configuration. + if len(accumulatedExclude) > 0 { + if result.CreatePullRequests != nil { + if excludeFiles, ok := accumulatedExclude["create-pull-request"]; ok && len(excludeFiles) > 0 { + result.CreatePullRequests.ProtectedFilesExclude = mergeUnique( + result.CreatePullRequests.ProtectedFilesExclude, + excludeFiles..., + ) + importsLog.Printf("Merged %d accumulated protected-files exclude(s) into create-pull-request", len(excludeFiles)) + } + } + if result.PushToPullRequestBranch != nil { + if excludeFiles, ok := accumulatedExclude["push-to-pull-request-branch"]; ok && len(excludeFiles) > 0 { + result.PushToPullRequestBranch.ProtectedFilesExclude = mergeUnique( + result.PushToPullRequestBranch.ProtectedFilesExclude, + excludeFiles..., + ) + importsLog.Printf("Merged %d accumulated protected-files exclude(s) into push-to-pull-request-branch", len(excludeFiles)) + } + } + } + importsLog.Printf("Successfully merged safe-outputs from imports") return result, nil } @@ -416,6 +456,13 @@ func mergeSafeOutputConfig(result *SafeOutputsConfig, config map[string]any, c * } if result.CreatePullRequests == nil && importedConfig.CreatePullRequests != nil { result.CreatePullRequests = importedConfig.CreatePullRequests + } else if result.CreatePullRequests != nil && importedConfig.CreatePullRequests != nil { + // Merge protected-files exclude lists as a set so that imports can extend exclusions + // without replacing the top-level configuration entirely. + result.CreatePullRequests.ProtectedFilesExclude = mergeUnique( + result.CreatePullRequests.ProtectedFilesExclude, + importedConfig.CreatePullRequests.ProtectedFilesExclude..., + ) } if result.CreatePullRequestReviewComments == nil && importedConfig.CreatePullRequestReviewComments != nil { result.CreatePullRequestReviewComments = importedConfig.CreatePullRequestReviewComments @@ -461,6 +508,13 @@ func mergeSafeOutputConfig(result *SafeOutputsConfig, config map[string]any, c * } if result.PushToPullRequestBranch == nil && importedConfig.PushToPullRequestBranch != nil { result.PushToPullRequestBranch = importedConfig.PushToPullRequestBranch + } else if result.PushToPullRequestBranch != nil && importedConfig.PushToPullRequestBranch != nil { + // Merge protected-files exclude lists as a set so that imports can extend exclusions + // without replacing the top-level configuration entirely. + result.PushToPullRequestBranch.ProtectedFilesExclude = mergeUnique( + result.PushToPullRequestBranch.ProtectedFilesExclude, + importedConfig.PushToPullRequestBranch.ProtectedFilesExclude..., + ) } if result.UploadAssets == nil && importedConfig.UploadAssets != nil { result.UploadAssets = importedConfig.UploadAssets diff --git a/pkg/workflow/push_to_pull_request_branch.go b/pkg/workflow/push_to_pull_request_branch.go index 85738d1a499..fef6ca01b8c 100644 --- a/pkg/workflow/push_to_pull_request_branch.go +++ b/pkg/workflow/push_to_pull_request_branch.go @@ -21,6 +21,7 @@ type PushToPullRequestBranchConfig struct { TargetRepoSlug string `yaml:"target-repo,omitempty"` // Target repository in format "owner/repo" for cross-repository push to pull request branch AllowedRepos []string `yaml:"allowed-repos,omitempty"` // List of additional repositories in format "owner/repo" that push to pull request branch can target ManifestFilesPolicy *string `yaml:"protected-files,omitempty"` // Controls protected-file protection: "blocked" (default) hard-blocks, "allowed" permits all changes, "fallback-to-issue" creates a review issue instead of pushing. + ProtectedFilesExclude []string `yaml:"-"` // Files/prefixes to exclude from the default protected list (from object-form protected-files.exclude). Not sourced from YAML directly; populated during parsing. AllowedFiles []string `yaml:"allowed-files,omitempty"` // Strict allowlist of glob patterns for files eligible for push. Checked independently of protected-files; both checks must pass. ExcludedFiles []string `yaml:"excluded-files,omitempty"` // List of glob patterns for files to exclude from the patch using git :(exclude) pathspecs. Matching files are stripped by git at generation time and will not appear in the commit or be subject to allowed-files or protected-files checks. PatchFormat string `yaml:"patch-format,omitempty"` // Transport format for packaging changes: "am" (default, uses git format-patch) or "bundle" (uses git bundle, preserves merge topology and per-commit metadata). @@ -139,7 +140,10 @@ func (c *Compiler) parsePushToPullRequestBranchConfig(outputMap map[string]any) // Parse allowed-repos for cross-repository push pushToBranchConfig.AllowedRepos = parseAllowedReposFromConfig(configMap) - // Parse protected-files: pure string enum ("blocked", "allowed", "fallback-to-issue"). + // Parse protected-files: supports string enum OR object form {policy, exclude}. + exclude := preprocessProtectedFilesField(configMap, pushToPullRequestBranchLog) + pushToBranchConfig.ProtectedFilesExclude = exclude + // Validate policy string (no-op if the field was replaced by preprocessor) manifestFilesEnums := []string{"blocked", "allowed", "fallback-to-issue"} validateStringEnumField(configMap, "protected-files", manifestFilesEnums, pushToPullRequestBranchLog) if strVal, ok := configMap["protected-files"].(string); ok { diff --git a/pkg/workflow/runtime_definitions.go b/pkg/workflow/runtime_definitions.go index 97184478fc6..dd9502baf0d 100644 --- a/pkg/workflow/runtime_definitions.go +++ b/pkg/workflow/runtime_definitions.go @@ -216,6 +216,26 @@ func getProtectedPathPrefixes(extra ...string) []string { return mergeUnique([]string{".github/", ".agents/"}, extra...) } +// excludeFromSlice returns a new slice containing the items from base +// that do not appear in the exclude set. Order of remaining items is preserved. +// Always returns a fresh slice (never aliases base) even when no items are removed. +func excludeFromSlice(base []string, exclude ...string) []string { + if len(exclude) == 0 { + return append([]string(nil), base...) + } + excluded := make(map[string]bool, len(exclude)) + for _, v := range exclude { + excluded[v] = true + } + result := make([]string, 0, len(base)) + for _, v := range base { + if !excluded[v] { + result = append(result, v) + } + } + return result +} + // mergeUnique returns a deduplicated slice that starts with base and appends any // items from extra that are not already present in base. Order is preserved. func mergeUnique(base []string, extra ...string) []string { diff --git a/pkg/workflow/safe_outputs_config_generation.go b/pkg/workflow/safe_outputs_config_generation.go index 50695b278a5..b25fd9b0ae5 100644 --- a/pkg/workflow/safe_outputs_config_generation.go +++ b/pkg/workflow/safe_outputs_config_generation.go @@ -37,6 +37,9 @@ func generateSafeOutputsConfig(data *WorkflowData) (string, error) { // Standard handler configs — sourced from handlerRegistry (same as GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG) for handlerName, builder := range handlerRegistry { if handlerCfg := builder(data.SafeOutputs); handlerCfg != nil { + // Strip the internal sentinel key used by the handler manager for compile-time + // exclusion processing — it must not be forwarded to the runtime config.json. + delete(handlerCfg, "_protected_files_exclude") safeOutputsConfig[handlerName] = handlerCfg } } diff --git a/pkg/workflow/safe_outputs_config_helpers.go b/pkg/workflow/safe_outputs_config_helpers.go index 869b7f81ff4..c054663dde3 100644 --- a/pkg/workflow/safe_outputs_config_helpers.go +++ b/pkg/workflow/safe_outputs_config_helpers.go @@ -208,3 +208,17 @@ func buildCustomSafeOutputJobsJSON(data *WorkflowData) string { } return string(jsonBytes) } + +// extractStringSliceFromConfig retrieves a []string value from a handler config map. +// It gracefully handles both []string and []any element types. Returns nil when the +// key is absent or the value cannot be coerced. +func extractStringSliceFromConfig(config map[string]any, key string) []string { + if config == nil { + return nil + } + raw, exists := config[key] + if !exists || raw == nil { + return nil + } + return parseStringSliceAny(raw, nil) +} diff --git a/pkg/workflow/safe_outputs_fix_test.go b/pkg/workflow/safe_outputs_fix_test.go index 0570a9f729a..cdc78451501 100644 --- a/pkg/workflow/safe_outputs_fix_test.go +++ b/pkg/workflow/safe_outputs_fix_test.go @@ -256,3 +256,78 @@ func TestMergeSafeOutputsMetaFieldsUnit(t *testing.T) { }) } } + +// TestMergeProtectedFilesExcludeAsSet verifies that when both the top-level and imported +// configs define the same handler, their protected-files exclude lists are merged as a set. +func TestMergeProtectedFilesExcludeAsSet(t *testing.T) { + compiler := NewCompilerWithVersion("1.0.0") + + tests := []struct { + name string + topConfig *SafeOutputsConfig + imported string + verify func(t *testing.T, result *SafeOutputsConfig) + }{ + { + name: "create-pull-request exclude merged from import when top has none", + topConfig: &SafeOutputsConfig{ + CreatePullRequests: &CreatePullRequestsConfig{}, + }, + imported: `{"create-pull-request":{"protected-files":{"exclude":["AGENTS.md"]}}}`, + verify: func(t *testing.T, result *SafeOutputsConfig) { + require.NotNil(t, result.CreatePullRequests, "CreatePullRequests should be present") + assert.Equal(t, []string{"AGENTS.md"}, result.CreatePullRequests.ProtectedFilesExclude, + "Imported exclude should be merged") + }, + }, + { + name: "create-pull-request exclude merged as set when both define it", + topConfig: &SafeOutputsConfig{ + CreatePullRequests: &CreatePullRequestsConfig{ + ProtectedFilesExclude: []string{"CLAUDE.md"}, + }, + }, + imported: `{"create-pull-request":{"protected-files":{"exclude":["AGENTS.md"]}}}`, + verify: func(t *testing.T, result *SafeOutputsConfig) { + require.NotNil(t, result.CreatePullRequests, "CreatePullRequests should be present") + assert.ElementsMatch(t, []string{"CLAUDE.md", "AGENTS.md"}, result.CreatePullRequests.ProtectedFilesExclude, + "Exclude lists should be merged as a set") + }, + }, + { + name: "push-to-pull-request-branch exclude merged from import when top has none", + topConfig: &SafeOutputsConfig{ + PushToPullRequestBranch: &PushToPullRequestBranchConfig{}, + }, + imported: `{"push-to-pull-request-branch":{"protected-files":{"exclude":["AGENTS.md"]}}}`, + verify: func(t *testing.T, result *SafeOutputsConfig) { + require.NotNil(t, result.PushToPullRequestBranch, "PushToPullRequestBranch should be present") + assert.Equal(t, []string{"AGENTS.md"}, result.PushToPullRequestBranch.ProtectedFilesExclude, + "Imported exclude should be merged") + }, + }, + { + name: "deduplication: same item not added twice", + topConfig: &SafeOutputsConfig{ + CreatePullRequests: &CreatePullRequestsConfig{ + ProtectedFilesExclude: []string{"AGENTS.md"}, + }, + }, + imported: `{"create-pull-request":{"protected-files":{"exclude":["AGENTS.md","CLAUDE.md"]}}}`, + verify: func(t *testing.T, result *SafeOutputsConfig) { + require.NotNil(t, result.CreatePullRequests, "CreatePullRequests should be present") + assert.ElementsMatch(t, []string{"AGENTS.md", "CLAUDE.md"}, result.CreatePullRequests.ProtectedFilesExclude, + "Duplicate items should be deduplicated") + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := compiler.MergeSafeOutputs(tt.topConfig, []string{tt.imported}, nil) + require.NoError(t, err, "MergeSafeOutputs should not error") + require.NotNil(t, result, "result should not be nil") + tt.verify(t, result) + }) + } +} diff --git a/pkg/workflow/validation_helpers.go b/pkg/workflow/validation_helpers.go index ee57487de24..f78038a5946 100644 --- a/pkg/workflow/validation_helpers.go +++ b/pkg/workflow/validation_helpers.go @@ -116,6 +116,79 @@ func validateStringEnumField(configData map[string]any, fieldName string, allowe } } +// preprocessProtectedFilesField preprocesses the "protected-files" field in configData, +// handling both the legacy string-enum form and the new object form. +// +// String form (unchanged): "blocked", "allowed", or "fallback-to-issue". +// Object form: { policy: "blocked", exclude: ["AGENTS.md"] } +// - policy is optional; when missing or empty, this preprocessing step treats it as absent +// and leaves downstream default handling to apply (the "protected-files" key is deleted) +// - exclude is a list of filenames/path-prefixes to remove from the default protected set +// +// When the object form is encountered the field is normalised in-place: +// - "protected-files" is replaced with the extracted policy string, or deleted when policy is absent/empty +// - The extracted exclude slice is returned so callers can store it in the config struct +// +// When the string form is encountered the field is left unchanged and nil is returned. +// The log parameter is optional; pass nil to suppress debug output. +func preprocessProtectedFilesField(configData map[string]any, log *logger.Logger) []string { + if configData == nil { + return nil + } + raw, exists := configData["protected-files"] + if !exists || raw == nil { + return nil + } + pfMap, ok := raw.(map[string]any) + if !ok { + // String form — left for validateStringEnumField to handle + return nil + } + // Object form: extract policy and exclude + if policy, ok := pfMap["policy"].(string); ok && policy != "" { + configData["protected-files"] = policy + if log != nil { + log.Printf("protected-files object form: policy=%s", policy) + } + } else { + delete(configData, "protected-files") + if log != nil { + log.Print("protected-files object form: no policy, using default") + } + } + return parseStringSliceAny(pfMap["exclude"], log) +} + +// parseStringSliceAny coerces a raw any value into a []string. +// It accepts a []string (returned as-is), []any (string elements extracted), +// or nil (returns nil). The log parameter is optional; pass nil to suppress +// debug output about skipped non-string elements. +func parseStringSliceAny(raw any, log *logger.Logger) []string { + if raw == nil { + return nil + } + switch v := raw.(type) { + case []string: + // Already the right type — return directly without copying. + return v + case []any: + result := make([]string, 0, len(v)) + for _, item := range v { + if s, ok := item.(string); ok { + result = append(result, s) + } else if log != nil { + log.Printf("parseStringSliceAny: skipping non-string item: %T", item) + } + } + return result + default: + if log != nil { + log.Printf("parseStringSliceAny: unexpected type %T, ignoring", raw) + } + return nil + } +} + // validateNoDuplicateIDs checks that all items have unique IDs extracted by idFunc. // The onDuplicate callback creates the error to return when a duplicate is found. func validateNoDuplicateIDs[T any](items []T, idFunc func(T) string, onDuplicate func(string) error) error { diff --git a/pkg/workflow/validation_helpers_test.go b/pkg/workflow/validation_helpers_test.go index 86e2ca00403..3c653137f6c 100644 --- a/pkg/workflow/validation_helpers_test.go +++ b/pkg/workflow/validation_helpers_test.go @@ -541,3 +541,106 @@ func TestContainsTrigger(t *testing.T) { }) } } + +// TestPreprocessProtectedFilesField tests the preprocessProtectedFilesField helper. +func TestPreprocessProtectedFilesField(t *testing.T) { + tests := []struct { + name string + configData map[string]any + wantExclude []string + wantPFAfter any // expected value of configData["protected-files"] after preprocessing + wantPFPresent bool // whether configData["protected-files"] should exist after preprocessing + }{ + { + name: "string form passes through unchanged", + configData: map[string]any{"protected-files": "blocked"}, + wantExclude: nil, + wantPFAfter: "blocked", + wantPFPresent: true, + }, + { + name: "string form allowed passes through unchanged", + configData: map[string]any{"protected-files": "allowed"}, + wantExclude: nil, + wantPFAfter: "allowed", + wantPFPresent: true, + }, + { + name: "object form with policy and exclude", + configData: map[string]any{ + "protected-files": map[string]any{ + "policy": "fallback-to-issue", + "exclude": []any{"AGENTS.md", "CLAUDE.md"}, + }, + }, + wantExclude: []string{"AGENTS.md", "CLAUDE.md"}, + wantPFAfter: "fallback-to-issue", + wantPFPresent: true, + }, + { + name: "object form with exclude only (no policy)", + configData: map[string]any{ + "protected-files": map[string]any{ + "exclude": []any{"AGENTS.md"}, + }, + }, + wantExclude: []string{"AGENTS.md"}, + wantPFPresent: false, // key removed when no policy + }, + { + name: "object form with empty policy string", + configData: map[string]any{ + "protected-files": map[string]any{ + "policy": "", + "exclude": []any{"AGENTS.md"}, + }, + }, + wantExclude: []string{"AGENTS.md"}, + wantPFPresent: false, // empty policy treated as absent + }, + { + name: "nil configData returns nil", + configData: nil, + wantExclude: nil, + wantPFPresent: false, + }, + { + name: "absent field returns nil", + configData: map[string]any{"other": "value"}, + wantExclude: nil, + wantPFPresent: false, + }, + { + name: "object form with empty exclude list", + configData: map[string]any{ + "protected-files": map[string]any{ + "policy": "blocked", + "exclude": []any{}, + }, + }, + wantExclude: nil, + wantPFAfter: "blocked", + 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") + } + + if tt.configData == nil { + return + } + 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") + } + }) + } +}