Normalize PR protected-file policy defaults and changelog handling#56562
Conversation
Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
|
Re this: I thought it was Why was this given as an example in PR description? |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "proxy.golang.org"See 网络 Configuration for more information.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
Warning Firewall blocked 4 domainsThe following domains were blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"
- "api.github.com"
- "chatgpt.com"
- "github.com"See 网络 Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
There was a problem hiding this comment.
Pull request overview
Normalizes protected-file policy defaults across pull-request safe-output handlers and relaxes default changelog protection.
Changes:
- Canonicalizes
request-reviewwhile retaining runtime alias handling. - Excludes
CHANGELOG.mdfrom PR-handler protection defaults. - Updates validation, documentation, tests, and generated fixtures.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/with-imports.golden |
Refreshes generated environment exports. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden |
Refreshes generated environment exports. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/playwright-cli-mode.golden |
Refreshes generated environment exports. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/basic-copilot.golden |
Refreshes generated environment exports. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/gemini.golden |
Refreshes Gemini golden output. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/copilot.golden |
Refreshes Copilot golden output. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/codex.golden |
Refreshes Codex golden output. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/claude.golden |
Refreshes Claude golden output. |
pkg/workflow/safe_outputs_handler_registry_pull_requests.go |
Unifies policy defaults and changelog exclusions. |
pkg/workflow/runtime_definitions.go |
Removes changelog from shared protection. |
pkg/workflow/push_to_pull_request_branch.go |
Parses and normalizes review policies. |
pkg/workflow/create_pull_request.go |
Accepts the canonical policy spelling. |
pkg/workflow/compiler_safe_outputs_config_protection_test.go |
Updates protection expectations. |
pkg/workflow/compiler_safe_outputs_config_pr_policy_test.go |
Tests hyphenated handler defaults. |
pkg/parser/schemas/main_workflow_schema.json |
Updates create-PR policy schema. |
actions/setup/md/manifest_protection_request_review.md |
Updates user-facing policy naming. |
actions/setup/js/manifest_file_helpers.cjs |
Handles canonical and legacy runtime values. |
.github/aw/safe-outputs-management.md |
Documents the push-handler default. |
.github/aw/safe-outputs-content.md |
Documents the create-PR default. |
Review details
Suppressed comments (1)
pkg/parser/schemas/main_workflow_schema.json:7730
- Object-form configurations have the same compatibility break:
policy: request_reviewis rejected by schema validation, and the schema default still uses the legacy spelling that was removed from its own enum. Accept the alias while exposingrequest-reviewas the default.
"enum": ["blocked", "allowed", "fallback-to-issue", "request-review"],
"description": "Protection policy. request-review (default): create the PR but prepend a caution block and submit a REQUEST_CHANGES review. blocked: 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": "request_review"
- Files reviewed: 19/19 changed files
- Comments generated: 4
- Review effort level: Balanced
| manifestFilesEnums := []string{"blocked", "allowed", "fallback-to-issue", "request_review", "request-review"} | ||
| validateStringEnumField(configMap, "protected-files", manifestFilesEnums, pushToPullRequestBranchLog) | ||
| if strVal, ok := configMap["protected-files"].(string); ok { | ||
| pushToBranchConfig.ManifestFilesPolicy = &strVal | ||
| normalised := normaliseProtectedFilesPolicy(strVal) | ||
| pushToBranchConfig.ManifestFilesPolicy = &normalised |
| @@ -208,7 +208,6 @@ var securityConfigFiles = []string{ | |||
| "DESIGN.md", // Captures design-system source of truth consumed by coding agents | |||
| "README.md", // Primary documentation file often imported by agents as context | |||
| "CONTRIBUTING.md", // Contribution guidelines; modifying could mislead contributors or agents | |||
| **Allowed Source 分支**: When `allowed-branches` is set, the branch used for PR creation (agent-provided `branch` or the current checkout branch when omitted) must match one of the configured glob patterns. | ||
|
|
||
| **File Restrictions**: **Always specify `allowed-files`** — this is the primary guardrail for `create-pull-request`. Scope it to specific file extensions (e.g., `"**/*.md"`, `"**/*.ts"`) or directory paths (e.g., `"src/**"`, `"docs/**"`) matching the workflow's purpose. Omitting `allowed-files` allows the agent to touch any file in the repository, which significantly expands blast radius. Use `excluded-files` to additionally strip specific files (e.g. lock files) from the patch before any checks. The `protected-files` field controls handling of sensitive files (package manifests, CI configs, agent instruction files): `request_review` (default — create the PR but submit a `REQUEST_CHANGES` review so a human approves before merge), `blocked` (hard-block), `fallback-to-issue` (push branch and create a review issue), or `allowed` (no restriction — use only when the workflow is explicitly designed to manage these files). Object form is also supported: `protected-files: { policy: fallback-to-issue, exclude: [AGENTS.md] }`. | ||
| **File Restrictions**: **Always specify `allowed-files`** — this is the primary guardrail for `create-pull-request`. Scope it to specific file extensions (e.g., `"**/*.md"`, `"**/*.ts"`) or directory paths (e.g., `"src/**"`, `"docs/**"`) matching the workflow's purpose. Omitting `allowed-files` allows the agent to touch any file in the repository, which significantly expands blast radius. Use `excluded-files` to additionally strip specific files (e.g. lock files) from the patch before any checks. The `protected-files` field controls handling of sensitive files (package manifests, CI configs, agent instruction files): `request-review` (default — create the PR but submit a `REQUEST_CHANGES` review so a human approves before merge), `blocked` (hard-block), `fallback-to-issue` (push branch and create a review issue), or `allowed` (no restriction — use only when the workflow is explicitly designed to manage these files). Object form is also supported: `protected-files: { policy: fallback-to-issue, exclude: [AGENTS.md] }`. |
🏗️ ADR Required — draft added for PR #56562I found no ADR referenced in the PR body, and the branch ADR set did not contain one for this change before review. Because this PR exceeds the default implementation threshold ( Evidence used
Action taken
Next action for the author
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "proxy.golang.org"See 网络 Configuration for more information.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
REQUEST_CHANGES
The protected-files default has been changed in code, but the contract is still inconsistent and the policy change is under-tested.
Blocking themes
push-to-pull-request-branchstill documents and validatesblockedas the default in the schema, while the compiler now emitsrequest-reviewthere.- Excluding
CHANGELOG.mdfrom the default protected set changes the security boundary, but the PR only adds compile-time config assertions and does not add runtime enforcement coverage for the new behavior.
Those need to be reconciled before this lands; otherwise users get a silent default change plus a contract/runtime mismatch.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 41.9 AIC · ⌖ 7.5 AIC · ⊞ 7K
Comment /review to run again
| AddIfNotEmpty("github-token", resolveHandlerGitHubToken(c.GitHubApp, "push-to-pull-request-branch", c.GitHubToken)). | ||
| AddTemplatableBool("staged", templatableBoolPtrToStringPtr(c.Staged)). | ||
| AddStringPtr("protected_files_policy", c.ManifestFilesPolicy). | ||
| AddDefault("protected_files_policy", protectedFilesPolicy). |
There was a problem hiding this comment.
This change only updates the create-pull-request schema/docs to make request-review the default. The push-to-pull-request-branch schema block is still documented and validated as blocked-by-default (pkg/parser/schemas/main_workflow_schema.json around the 9966 section), while the compiler now emits request-review there too. That leaves the published contract out of sync with runtime behavior and silently weakens a protection boundary for callers that rely on the documented default.
💡 Why this matters and how to fix it
Right now three layers disagree:
# docs/schema for push-to-pull-request-branch still imply:
protected-files: blocked
# compiler now emits:
protected_files_policy: request-reviewThat is not a cosmetic mismatch. Consumers validating against the schema or reading the docs will believe pushes to PR branches hard-fail on protected files unless they opt into a softer mode, but the generated handler config now does the softer thing by default.
Please either:
- keep
push-to-pull-request-branchdefaulting toblocked, or - update the schema, tests, and user-facing docs for that handler in the same PR so the contract matches the generated runtime config.
| @@ -208,7 +208,6 @@ var securityConfigFiles = []string{ | |||
| "DESIGN.md", // Captures design-system source of truth consumed by coding agents | |||
There was a problem hiding this comment.
Dropping CHANGELOG.md from the default protected-file set is a behavior change with real security impact, but this PR only adjusts compiler-side tests. I can't find a matching runtime test that proves changelog-only patches are now intentionally allowed while other protected top-level docs still trigger protection. Without that coverage, this is one refactor away from becoming an accidental policy regression.
💡 What coverage is missing
The new Go tests only assert the generated config shape. They do not exercise the JS policy evaluator that actually decides whether a patch is blocked / fallback / request-review.
Please add a runtime-level test around checkFileProtection / checkFileProtectionPostApply showing at least:
// CHANGELOG is no longer protected by default
CHANGELOG.md -> allow
// adjacent sensitive docs still remain protected
README.md -> request_review or deny
.github/x.yml -> request_review or denyThat makes the intended policy change explicit and prevents a future mismatch between compile-time config generation and enforcement.
There was a problem hiding this comment.
Added runtime-level coverage in actions/setup/js/manifest_file_helpers.test.cjs: CHANGELOG.md is allowed with the PR handler's filtered protected-file set, while README.md still produces request_review under request-review policy. The shared manifest list continues to protect changelogs for other handlers.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — light COMMENT with a few targeted observations.
📋 Key Themes & Highlights
Key Themes
- Dead normalisation branch:
normaliseProtectedFilesPolicyhandlesfallback_to_issue(underscore) but that value is never accepted by any enum validator, making the branch unreachable. Either remove it or add it to the accepted enums to make the compatibility intent explicit. - Hardcoded default excludes not advertised:
defaultProtectedFilesExcludesilently prependsCHANGELOG.mdwith no escape hatch. A small comment or named constant would clarify this is intentional and permanent. - Fragile test extraction: The new
TestProtectedFilesPolicyDefaultsAreHyphenateddeserialises config by string-splitting an env-var line, coupling the test to an internal serialisation detail. A light comment or a dedicated helper would improve resilience.
Positive Highlights
- ✅ Solid backward-compatibility story:
request_review(underscore) is still accepted and silently normalised — old configs won't break. - ✅ Both
create-pull-requestandpush-to-pull-request-branchnow share the same default viapushToPullRequestBranchProtectedFilesPolicy, eliminating divergence. - ✅ Schema, docs, and golden tests are all updated in the same PR — no documentation drift.
- ✅
defaultProtectedFilesExcludededuplicates correctly with the seen-map pattern.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 35 AIC · ⌖ 10.8 AIC · ⊞ 7.6K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/safe_outputs_handler_registry_pull_requests.go:337
[/codebase-design] The fallback_to_issue → fallback-to-issue branch in normaliseProtectedFilesPolicy is dead code — fallback_to_issue (underscore) was never a valid enum value in the accepted lists in create_pull_request.go or push_to_pull_request_branch.go, so it can never reach this function. This implies silent normalization of an otherwise-invalid value, which may mislead future readers.
<details>
<summary>💡 Suggestion</summary>
Either remove the fallback_to_issue case,…
pkg/workflow/safe_outputs_handler_registry_pull_requests.go:344
[/codebase-design] The defaultProtectedFilesExclude function prepends CHANGELOG.md by prepending it to the excludes slice via append([]string{"CHANGELOG.md"}, excludes...). This means if excludes already contains CHANGELOG.md (e.g. from an explicit user config), the deduplication loop handles it, but the intent is slightly obscured. More importantly, the default inject is invisible to callers — there is no way to opt out of having CHANGELOG.md excluded once this function is …
pkg/workflow/compiler_safe_outputs_config_pr_policy_test.go:157
[/tdd] The new test TestProtectedFilesPolicyDefaultsAreHyphenated extracts the JSON config by string-splitting on GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG: — this is fragile coupling to the env-var serialization format. If the config key or surrounding whitespace ever changes, the test silently fails with a misleading "should have extracted JSON" error rather than a clear signal about what broke.
<details>
<summary>💡 Suggestion</summary>
Either expose a testable helper that returns the par…
There was a problem hiding this comment.
One compatibility shim can be dropped to keep the policy handling simpler. net: -1 lines possible.
Warning
Firewall blocked 4 domains
The following domains were blocked by the firewall during workflow execution:
ab.chatgpt.comapi.github.comchatgpt.comgithub.com
[!TIP]
api.github.com is blocked because GitHub API access uses the built-in GitHub tools by default. Instead of adding api.github.com to network.allowed, use tools.github.mode: gh-proxy for direct pre-authenticated GitHub CLI access without requiring network access to api.github.com:
tools:
github:
mode: gh-proxySee GitHub Tools for more information on gh-proxy mode.
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"
- "api.github.com"
- "chatgpt.com"
- "github.com"See 网络 Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #56562 · codex · mai10 · 6.8 AIC · ⌖ 0.534 AIC · ⊞ 12.8K
Comment /ponytail to run again
| AddTemplatableBool("staged", templatableBoolPtrToStringPtr(c.Staged)) | ||
| } | ||
|
|
||
| func normaliseProtectedFilesPolicy(policy string) string { |
There was a problem hiding this comment.
pkg/workflow/safe_outputs_handler_registry_pull_requests.go:295: shrink: compatibility branches for both request_review and request-review. Standardize on one canonical request-review value and drop the alias branch.
There was a problem hiding this comment.
Retained the compatibility branch intentionally. Existing workflows using the legacy public request_review value must continue to work; schema validation accepts it and the runtime normalizes it to canonical request-review.
There was a problem hiding this comment.
Two issues found:
-
Schema
defaultmismatch (blocking) —main_workflow_schema.jsonstill declares"default": "request_review"(underscore) in two places (lines 7714 and 7730), but"request_review"was removed from theenum. The default is now an invalid value per the schema's own constraints. -
安全 posture regression (non-blocking, needs rationale) —
push-to-pull-request-branchdefault policy was silently downgraded fromblocked→request-review. If intentional, this change should be documented with explicit rationale and flagged as a breaking change for operators relying on the stricter default.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 57.7 AIC · ⌖ 10.8 AIC · ⊞ 6.2K
| "description": "Controls protected-file protection. request_review (default): create the PR but prepend a caution block and submit a REQUEST_CHANGES review for manual scrutiny. blocked: 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.", | ||
| "enum": ["blocked", "allowed", "fallback-to-issue", "request-review"], | ||
| "description": "Controls protected-file protection. request-review (default): create the PR but prepend a caution block and submit a REQUEST_CHANGES review for manual scrutiny. blocked: 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": "request_review" |
There was a problem hiding this comment.
The "default" value is still "request_review" (underscore), but "request_review" was removed from the enum in this PR — only "request-review" (hyphen) is now a valid enum member. A JSON Schema validator will flag this default as invalid.
Both occurrences (lines 7714 and 7730) need:
"default": "request-review"@copilot please address this.
There was a problem hiding this comment.
Fixed in the pushed schema update: both canonical request-review and legacy request_review are valid enum values, while the schema default is canonical request-review.
| @@ -291,12 +292,45 @@ func newCreatePullRequestHandlerConfigBuilder(cfg *SafeOutputsConfig, c *CreateP | |||
| AddTemplatableBool("staged", templatableBoolPtrToStringPtr(c.Staged)) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The default policy for push-to-pull-request-branch was changed from blocked to request-review in both the documentation example (.github/aw/safe-outputs-management.md) and the runtime default (pushToPullRequestBranchProtectedFilesPolicy).
This is a security-posture regression: push-to-pull-request-branch was intentionally more restrictive than create-pull-request because pushing to an existing PR branch can silently slip protected-file changes past reviewers who don't re-read the full diff. blocked forces humans to explicitly opt in to a looser policy; request-review is a softer default that agents may rely on opportunistically.
If the intent is to align both tools at request-review, the rationale should be explicitly documented (e.g. in the safe-outputs-management reference and in a CHANGELOG entry) so that operators who depended on the stricter default are aware of the change.
@copilot please address this.
There was a problem hiding this comment.
Kept the intended aligned default and documented the rationale in .github/aw/safe-outputs-management.md: request-review preserves the change and requires human approval, while blocked remains available for stricter workflows. The PR description/docs now call out the behavior change and the scoped CHANGELOG.md exception.
Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
You’re right—the PR description example used the internal/legacy field name incorrectly. The public configuration is safe-outputs:
create-pull-request:
protected-files: request-reviewor the object form you showed: safe-outputs:
create-pull-request:
protected-files:
policy: request-review
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the remaining blocking feedback and pushed the fixes. Added runtime protection coverage, documented the aligned |
|
@copilot There is still blocking review feedback on this PR. Please address the newest unresolved review threads, refresh the branch if needed, and then run the Current unresolved review threads, newest first:
I approved the waiting
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
The blocking feedback is addressed in the pushed commits: schema defaults are canonical |
|
@copilot Please refresh this branch if needed, review the latest feedback/check status, and then run the Run: https://github.com/github/gh-aw/actions/runs/33224630096
|
Reviewed the latest Sous Chef run and PR state. Run |
|
Category: refactor | Risk: medium | Score: 48 (impact 24/urgency 10/quality 14) | Action: batch_review | Batch: pr-batch:workflow-automation | CI: unknown
|
|
Category: refactor | Risk: medium | Score: 48 (impact 24/urgency 10/quality 14) | Action: batch_review | Batch: pr-batch:workflow-automation
|
|
@copilot Please refresh this branch if needed, verify the latest review and check state on the current head commit, and then run the Run: https://github.com/github/gh-aw/actions/runs/33234948407
|
Verified the latest Sous Chef run |
|
@copilot Please refresh this PR for forward progress. Re-run the
|
|
Category: refactor | Risk: medium | Score: 56/100 (impact 24/50, urgency 18/30, quality 14/20) | Action: batch_review | Batch: workflow-safety | Note: Policy normalization in shared safe-output behavior with meaningful but contained scope.
|
Reviewed the latest PR state and Sous Chef run |
The PR-safe-output policy default was inconsistent across handlers: the public surface used
request_revieweven though the canonical naming is hyphenated, and the default protected-file policy was stricter than needed for normal changelog edits. This madecreate-pull-requestandpush-to-pull-request-branchdiverge from the idiomatic config shape and blocked realistic changelog-only PRs by default.Summary
request-reviewwhile accepting the legacy underscore alias for compatibility.create-pull-requestandpush-to-pull-request-branch.CHANGELOG.mdfrom default protected-file enforcement so routine release updates do not require bypassing protection.Changes
create-pull-requestandpush-to-pull-request-branchrequest_reviewandrequest-reviewto the same canonical value:request-review.CHANGELOG.mdto the default exclusions used for PR manifest protection.request-reviewas the default public value.This keeps the public YAML surface idiomatic while not breaking older configs that still use the underscore form.