Add allow-workflows field for GitHub App workflows:write permission on safe-outputs

[Copilot]

Applies the changes from #25776 to current HEAD.

When allowed-files targets .github/workflows/ paths, pushing requires workflows:write — a GitHub App-only permission the compiler never inferred. Users resorted to fragile sed injection post-compile.

Changes

• New field allow-workflows: bool on create-pull-request and push-to-pull-request-branch configs
• Permission computation: ComputePermissionsForSafeOutputs adds workflows: write when allow-workflows: true (respects staged mode — no-op when staged)
• Compile-time validation: validateSafeOutputsAllowWorkflows errors when allow-workflows: true without a configured GitHub App (app-id + private-key)
• JSON schema: Both handler schemas updated with the new boolean field
• Tests: 6 test cases covering permissions, validation, parsing, and compile-error integration
• ADR-0002: Documents the explicit opt-in design decision (rejected auto-inference from allowed-files patterns for auditability)
• Docs: Updated safe-outputs-pull-requests.md and frontmatter-full.md

Usage

safe-outputs:
  github-app:
    app-id: ${{ vars.APP_ID }}
    private-key: ${{ secrets.APP_PRIVATE_KEY }}
  create-pull-request:
    allow-workflows: true
    allowed-files:
      - ".github/workflows/*.lock.yml"
    protected-files: allowed

[Copilot]

Pull request overview

Adds an explicit allow-workflows opt-in for safe-outputs PR handlers so the compiler can request the GitHub App-only workflows: write permission when users intend to modify files under .github/workflows/.

Changes:

• Introduces allow-workflows: bool on create-pull-request and push-to-pull-request-branch safe-outputs configs and threads it through parsing + docs + schema.
• Updates safe-outputs permission computation to add workflows: write when allow-workflows: true (and the handler isn’t staged).
• Adds compile-time validation + unit tests + an ADR documenting the explicit opt-in decision.

Show a summary per file

File	Description
pkg/workflow/safe_outputs_validation.go	Adds allow-workflows validation requiring a configured GitHub App.
pkg/workflow/safe_outputs_permissions.go	Adds workflows: write to computed permissions when allow-workflows is enabled.
pkg/workflow/safe_outputs_allow_workflows_test.go	New tests covering permissions, parsing, validation, and compile-time failure.
pkg/workflow/push_to_pull_request_branch.go	Adds AllowWorkflows field and parses allow-workflows from config.
pkg/workflow/create_pull_request.go	Adds AllowWorkflows field to config struct.
pkg/workflow/compiler.go	Wires the new safe-outputs validation into the compile pipeline.
pkg/parser/schemas/main_workflow_schema.json	Extends JSON schema for both handlers with allow-workflows.
docs/src/content/docs/reference/safe-outputs-pull-requests.md	Documents the new allow-workflows behavior and rationale.
docs/src/content/docs/reference/frontmatter-full.md	Regenerates frontmatter reference including allow-workflows.
docs/adr/0002-explicit-opt-in-allow-workflows-permission.md	Adds ADR documenting the explicit opt-in design.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 10/10 changed files
• Comments generated: 3

[Copilot]

The validation error message only interpolates the first handler found (handlers[0]) into both the config path and the example snippet. If both create-pull-request and push-to-pull-request-branch set allow-workflows: true, the error will misleadingly point to only one handler. Consider including all handler names in the error (e.g., join the list for the message, and/or emit one line per handler path) so users can quickly find every offending config block.

Suggested change

	return fmt.Errorf(
	"safe-outputs.%s.allow-workflows: requires a GitHub App to be configured.\n"+
	"The workflows permission is a GitHub App-only permission and cannot be granted via GITHUB_TOKEN.\n\n"+
	"Add a GitHub App configuration to safe-outputs:\n\n"+
	"safe-outputs:\n"+
	" github-app:\n"+
	" app-id: ${{ vars.APP_ID }}\n"+
	" private-key: ${{ secrets.APP_PRIVATE_KEY }}\n"+
	" %s:\n"+
	" allow-workflows: true",
	handlers[0], handlers[0],
	handlerPaths := make([]string, 0, len(handlers))
	handlerExamples := make([]string, 0, len(handlers))
	for _, handler := range handlers {
	handlerPaths = append(handlerPaths, fmt.Sprintf("safe-outputs.%s.allow-workflows", handler))
	handlerExamples = append(handlerExamples, fmt.Sprintf(" %s:\n allow-workflows: true", handler))
	}
	return fmt.Errorf(
	"%s: requires a GitHub App to be configured.\n"+
	"The workflows permission is a GitHub App-only permission and cannot be granted via GITHUB_TOKEN.\n\n"+
	"Add a GitHub App configuration to safe-outputs:\n\n"+
	"safe-outputs:\n"+
	" github-app:\n"+
	" app-id: ${{ vars.APP_ID }}\n"+
	" private-key: ${{ secrets.APP_PRIVATE_KEY }}\n"+
	"%s",
	strings.Join(handlerPaths, "\n"),
	strings.Join(handlerExamples, "\n"),

[Copilot]

This new validation is inserted after the network firewall validations, splitting the safe-outputs validation block (target/max/allowed-domains) into two separate sections. For maintainability and discoverability, consider keeping all safe-outputs validations together (e.g., moving this check up near the other safe-outputs validations).

[Copilot]

This ADR is numbered 0002, but docs/adr/0002-allow-secrets-in-step-env-bindings-under-strict-mode.md already exists. Having two ADR-0002 documents will cause ambiguity and broken references. Please renumber this new ADR (and its title/header) to the next available number (e.g., ADR-0003) to keep the sequence unique.

[github-actions]

✅ Design Decision Gate — ADR Verified

The implementation in this PR aligns with the stated Architecture Decision Record.

ADR reviewed: ADR-0002: Explicit Opt-In for GitHub App workflows:write Permission via allow-workflows Field

Verification Summary

All normative requirements from the ADR are satisfied by the code changes:

Requirement	Status	Evidence
allow-workflows field on both handlers	✅	create_pull_request.go:42, push_to_pull_request_branch.go:27
workflows: write added only when allow-workflows: true	✅	safe_outputs_permissions.go:134,143
Staged handlers are excluded from permission	✅	AllowWorkflows check is inside !isHandlerStaged() guard
No auto-inference from allowed-files patterns	✅	No such inference code exists
Compile-time validation requiring GitHub App	✅	safe_outputs_validation.go:316, wired in compiler.go:210
Error includes handler name + config example	✅	safe_outputs_validation.go:341-355
JSON schema updated for both handlers	✅	pkg/parser/schemas/main_workflow_schema.json (both entries)
Documentation updated	✅	safe-outputs-pull-requests.md + frontmatter-full.md
6 test cases covering all scenarios	✅	safe_outputs_allow_workflows_test.go

⚠️ Action Items for Follow-Up

Two issues should be addressed post-merge:

1. ADR numbering conflict: The repository now contains two ADR files with the 0002 prefix:

   • docs/adr/0002-allow-secrets-in-step-env-bindings-under-strict-mode.md
   • docs/adr/0002-explicit-opt-in-allow-workflows-permission.md

   One of these should be renumbered (likely this one to 0003) to restore sequential uniqueness. Suggested rename: docs/adr/0003-explicit-opt-in-allow-workflows-permission.md.

2. ADR status: The ADR footer says "This is a DRAFT ADR ... The PR author must review, complete, and finalize this document before the PR can merge." Now that this decision has been implemented and merged, the status should be updated from Draft to Accepted and the draft disclaimer footer removed.

The design decision has been recorded and the implementation faithfully follows it. 🏗️

🏗️ ADR gate enforced by Design Decision Gate 🏗️ · ● 120.4K · ◷

[github-actions]

Implementation verified: code aligns with the linked Architecture Decision Record (ADR-0002). All MUST/MUST NOT normative requirements are satisfied. See the inline comment for the full verification matrix and two minor follow-up items (ADR numbering conflict + status update).

[github-actions]

🧪 Test Quality Sentinel Report

Test Quality Score: 80/100

✅ Excellent test quality

Metric	Value
New/modified tests analyzed	6
✅ Design tests (behavioral contracts)	6 (100%)
⚠️ Implementation tests (low value)	0 (0%)
Tests with error/edge cases	4 (67%)
Duplicate test clusters	0
Test inflation detected	Yes (336 test lines vs 74 production lines, ~4.5x)
🚨 Coding-guideline violations	0

---

Test Classification Details

All 6 test functions

Test	File	Classification	Notes
TestAllowWorkflowsPermission	pkg/workflow/safe_outputs_allow_workflows_test.go:17	✅ Design	Table-driven (6 cases); covers staged/false/both-handlers edge cases
TestAllowWorkflowsValidationRequiresGitHubApp	pkg/workflow/safe_outputs_allow_workflows_test.go:97	✅ Design	Table-driven (7 cases); covers nil, empty GitHubApp, and error paths
TestAllowWorkflowsParsing	pkg/workflow/safe_outputs_allow_workflows_test.go:187	✅ Design	Verifies frontmatter parsing produces correct struct field; happy-path only
TestAllowWorkflowsParsingPushToPullRequestBranch	pkg/workflow/safe_outputs_allow_workflows_test.go:224	✅ Design	Same as above for the push-to-pull-request-branch handler; happy-path only
TestAllowWorkflowsAppTokenPermission	pkg/workflow/safe_outputs_allow_workflows_test.go:261	✅ Design	End-to-end: parse → compile → assert compiled YAML contains permission-workflows: write; happy-path only
TestAllowWorkflowsCompileErrorWithoutGitHubApp	pkg/workflow/safe_outputs_allow_workflows_test.go:302	✅ Design	Verifies compilation fails with actionable error when no GitHub App is configured

---

Flagged Tests — Minor Observations

None of the following rise to the level of blocking the PR — they are quality suggestions.

⚠️ TestAllowWorkflowsParsing & TestAllowWorkflowsParsingPushToPullRequestBranch (happy-path only)

Classification: Design test — but incomplete
Issue: Both parsing tests only verify that allow-workflows: true is correctly parsed. Neither includes a case where the field is absent or set to false (the default). Since the default value for a bool in Go is false, an unset field will always parse as false silently — a regression there would go undetected.
What design invariant does this test enforce? "When allow-workflows: true is present in frontmatter, the parsed struct reflects it." ✅
What would break if deleted? A regression in YAML key name or parser mapping would be missed. The absence of the false-default case means a silent-nil / missing-field regression would not be caught.
Suggested improvement: Add a sub-table or a second test case where allow-workflows is absent (or false) and assert AllowWorkflows == false. A table-driven refactor of these two functions would also eliminate the near-duplication.

⚠️ TestAllowWorkflowsAppTokenPermission (happy-path only)

Classification: Design test — but one-sided
Issue: Only asserts that permission-workflows: write is present in the compiled output. There is no complementary assertion verifying that when allow-workflows is not set the permission string is absent from the compiled YAML.
Suggested improvement: Add a second table row (or a sibling test) compiling a workflow without allow-workflows: true and asserting assert.NotContains(t, stepsStr, "permission-workflows: write", ...).

⚠️ Test inflation (4.5x ratio)

The test file adds 336 lines against ~74 lines of production changes across five files, a ~4.5x ratio (threshold: 2:1). This is partially explained by multi-line embedded YAML strings inside the test functions (each test includes a full workflow markdown literal). The quality of the tests is high, so this is informational only.

---

Language Support

Tests analyzed:

• 🐹 Go (*_test.go): 6 tests — unit (//go:build !integration) ✅

---

Verdict

✅ Check passed. 0% of new tests are implementation tests (threshold: 30%). All 6 tests verify observable behavioral contracts. Build tag is correctly present on line 1. No mock libraries used. All assertions include descriptive messages.

---

📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

• Assert on observable outputs, return values, or state changes
• Cover error paths and boundary conditions
• Would catch a behavioral regression if deleted
• Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

• Assert on internal function calls (mocking internals)
• Only test the happy path with typical inputs
• Break during legitimate refactoring even when behavior is correct
• Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.

🧪 Test quality analysis by Test Quality Sentinel · ● 582.3K · ◷

[github-actions]

✅ Test Quality Sentinel: 80/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All 6 tests verify observable behavioral contracts with proper build tags, no mock libraries, and descriptive assertion messages. Minor suggestions posted in the review comment.