Add safe output for approving fork pull request workflow runs - #52541
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds an opt-in safe output for approving fork pull-request workflow runs.
Changes:
- Adds compiler, schema, permission, and handler wiring.
- Implements guarded workflow-run approval with staged previews and limits.
- Adds TypeScript definitions, documentation, and tests.
Show a summary per file
| File | Description |
|---|---|
schemas/agent-output.json |
Defines approval output payload. |
pkg/workflow/unified_prompt_step.go |
Exposes the tool in prompts. |
pkg/workflow/safe_outputs_tools_computation.go |
Enables tool computation. |
pkg/workflow/safe_outputs_state.go |
Includes approval in state detection. |
pkg/workflow/safe_outputs_permissions_test.go |
Tests Actions write permission. |
pkg/workflow/safe_outputs_max_validation.go |
Validates approval limits. |
pkg/workflow/safe_outputs_handler_registry.go |
Builds runtime handler configuration. |
pkg/workflow/safe_outputs_config_types.go |
Adds Go configuration type. |
pkg/workflow/safe_outputs_config_extraction.go |
Extracts frontmatter configuration. |
pkg/workflow/safe_output_handlers.go |
Registers permissions and handler metadata. |
pkg/workflow/js/safe_outputs_tools.json |
Defines the compiler tool schema. |
pkg/workflow/compiler_safe_outputs_job.go |
Enables handler-manager generation. |
pkg/workflow/compiler_safe_outputs_config_test.go |
Tests configuration emission. |
pkg/workflow/approve_workflow_run.go |
Parses approval configuration. |
pkg/workflow/approve_workflow_run_test.go |
Tests configuration parsing. |
pkg/parser/schemas/main_workflow_schema.json |
Adds frontmatter schema support. |
docs/src/content/docs/reference/safe-outputs.md |
Adds the handler to the reference index. |
docs/src/content/docs/reference/safe-outputs-pull-requests.md |
Documents usage and permissions. |
actions/setup/js/types/safe-outputs.d.ts |
Adds approval output types. |
actions/setup/js/types/safe-outputs-config.d.ts |
Adds approval configuration types. |
actions/setup/js/safe_outputs_tools.json |
Defines the runtime MCP tool. |
actions/setup/js/safe_output_handler_manager.cjs |
Registers and classifies the handler. |
actions/setup/js/approve_workflow_run.test.cjs |
Tests validation, limits, and staging. |
actions/setup/js/approve_workflow_run.cjs |
Implements workflow-run approval. |
Review details
- Files reviewed: 22/24 changed files
- Comments generated: 2
- Review effort level: Balanced
🔍 PR TriageCategory: feature · Risk: high · Priority score: 62/100 (impact 30, urgency 12, quality 20) Score breakdown:
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel analysis complete: 80/100 Excellent. All test files analyzed (2 Go + 10 JS). No hard violations detected. Implementation ratio 50% (above 30% threshold) is justified for input validation. Budget preservation test validates commit 2. Comprehensive error coverage (8+ scenarios). Ready for APPROVE.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Reviewed for over-engineering only: approve_workflow_run.cjs, approve_workflow_run.go, and all wiring follow the existing safe-output handler pattern (config struct, registry entry, permission builder, tool schema) with no speculative abstractions, dead flexibility, or reinvented stdlib. No cuts identified.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
❌ Design Decision Gate 🏗️ failed during design decision gate check.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
This adds a dangerous new safe output, but the implementation still misses two blocking pieces: the approval-state check appears to use the wrong workflow-run field, and the runtime validator is not wired up for the new output type.
Blocking themes
- The handler can reject legitimately approval-blocked fork PR runs before ever calling GitHub's approval API.
- The new output type bypasses the normal safe-output field validation path, so malformed agent payloads are accepted further than they should be.
Both issues undercut the safety story for introducing a privileged mutation tool.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 15.2 AIC · ⌖ 4.7 AIC · ⊞ 6.5K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on a correctness bug and its mirrored test fixture.
📋 Key Themes & Highlights
Key Issues
- Critical correctness bug (
approve_workflow_run.cjs:73): The guard checksrun.conclusion !== "action_required", but GitHub returnsconclusion: nullwithstatus: "action_required"for runs awaiting fork approval. This means every eligible run will be incorrectly rejected. Fix: checkrun.status !== "action_required". - Test fixture embeds the bug (
approve_workflow_run.test.cjs:136):pendingPullRequestRunsetsconclusion: "action_required"instead ofstatus: "action_required", conclusion: null. The tests pass precisely because they mirror the wrong shape — they will not catch the bug at runtime. - Staged dry-run consumes max budget (
approve_workflow_run.cjs:79):processedCount++fires before theisStagedbranch, so preview calls count against the real limit. - Staged path requires
actions: read(pre-existing flag from #3778029017):getWorkflowRunis called unconditionally before the staged guard, requiring a read permission that may not be present on narrow staged tokens.
Positive Highlights
- ✅ Comprehensive test coverage for edge cases (invalid IDs, max enforcement, non-PR runs)
- ✅ Good threat-detection classification as a non-reviewable mutation
- ✅ Consistent wiring through the full compiler and permission stack
- ✅ Clean schema definitions in both TypeScript and JSON
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 42.1 AIC · ⌖ 6.96 AIC · ⊞ 7K
Comment /matt to run again
🧪 Test Quality Sentinel ReportQuality Score: 80/100 ✅ Excellent OverviewPR #52541 introduces comprehensive test coverage for the new Key Metrics
Test CoverageGo Tests (2 functions):
JavaScript Tests (8 + 1 parameterized = 10 scenarios):
Quality Assessment✅ Strengths:
Recommendation✅ APPROVE — Strong test quality. No hard violations. Implementation ratio justified for user-facing handler APIs where input validation is safety-critical. All tests have clear business value. Budget preservation invariant properly tested.
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 80/100 Excellent. Implementation tests: 50% (threshold ≤30%). High ratio justified — input validation and error messages are safety-critical for user-facing handler APIs. All tests prevent real regressions. No hard violations. Budget preservation test validates commit 2.
There was a problem hiding this comment.
The handler logic contains a critical correctness bug that would prevent any workflow run from ever being approved.
Blocking Issue
approve_workflow_run.cjs line 67 checks run.conclusion !== "action_required", but GitHub's REST API sets status: "waiting" (with conclusion: null) on workflow runs that are pending approval for fork PRs. The action_required conclusion is a check-run concept, not a workflow-run concept. As a result the handler always rejects valid runs and the feature is inoperable.
The test fixture (pendingPullRequestRun) hard-codes the same wrong value, so the tests pass but they don't reflect the real API response.
Fix required
- Change the eligibility guard to
run.status !== "waiting" - Update error message to report
status(notconclusion) - Update the test fixture to use
status: "waiting", conclusion: null
Rest of the PR
The overall structure (handler factory pattern, staged mode, max-count guard, parseRunId, Go-side config extraction, permissions wiring, schema update) is well-executed and consistent with the existing codebase patterns. Once the status/conclusion bug is corrected this should be ready to merge.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 48.5 AIC · ⌖ 6.54 AIC · ⊞ 5.4K
Addressed the actionable review items in b32f118: approval API failures release the reserved |
|
@copilot Quick triage for maintainer-ready follow-up: Please refresh the branch if GitHub allows it and run the The latest visible author update says the actionable review items from Please do this before handing back:
Failed checks from the compact candidate set:
Run context: https://github.com/github/gh-aw/actions/runs/31907553336
|
At |
|
@copilot define a behavior that allows "allow all workflow run for pr ..." where PR is current context or from allowed-pull-requests |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in |
|
@copilot the configuration must also specify a list of "allowed-workflows" so that only select workflow types are allowed to be run in this mode. Match the file name, normalize extension. Compiler should attempt to check. Support wildcards. |
|
@copilot Quick triage for maintainer-ready follow-up: Please refresh the branch if needed, address the remaining maintainer-facing follow-up, and run the Outstanding review items (newest first):
Failed checks from the compact candidate set:
Branch update was requested automatically for this run when GitHub allows it.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in |
Implemented in |
|
@copilot Quick triage for maintainer-ready follow-up: Please refresh the branch if GitHub allows it, address the remaining maintainer-facing follow-up, and run the Outstanding review items (newest first):
Branch update was requested automatically for this run when GitHub allows it.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the JS lint failure in |
|
/review |
|
✅ PR Code Quality Reviewer completed the code quality review.
|
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.
Verdict: request changes
The new safe output is close, but there is still one correctness hole in config validation: malformed allowed-workflows patterns can slip through compilation and then fail only at runtime.
Themes
allowed-workflowsvalidation is usingpath.Match(pattern, ""), which does not reliably reject bad glob syntax when the pattern can validly match an empty string. That means some malformed configs are accepted by the compiler and only blow up later inside the JS handler.- I did not find another changed-line issue strong enough to block merge.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 27.4 AIC · ⌖ 7.53 AIC · ⊞ 6.9K
Comment /review to run again
| if path.Base(pattern) != pattern { | ||
| return fmt.Errorf("safe-outputs.approve-workflow-run.allowed-workflows: %q must match a workflow filename, not a path", pattern) | ||
| } | ||
| if _, err := path.Match(normalizeApproveWorkflowRunAllowedWorkflowPattern(pattern), ""); err != nil { |
There was a problem hiding this comment.
This validation is unsound: path.Match(normalizeApproveWorkflowRunAllowedWorkflowPattern(pattern), "") only proves whether the pattern can be compiled against an empty candidate, so malformed allowed-workflows entries can still pass compilation and then explode later when the JS handler calls minimatch at runtime.
💡 Why this blocks merge
The contract in the docs and tests says the compiler rejects invalid wildcard syntax up front, but this check is using the wrong API shape for that. path.Match validates while also matching, and using "" as the candidate is not a reliable syntax-only probe because some broken patterns are only exposed when matching a non-empty path. That leaves you with a workflow that compiles successfully but fails only when an agent tries to approve a run.
Please switch this to a real syntax validation path that does not depend on the candidate string, and keep the regression test on genuinely malformed patterns.
candidate := normalizeApproveWorkflowRunAllowedWorkflowPattern(pattern)
if _, err := doublestar.Match(candidate, "x"); err != nil { // or another dedicated glob validator
return fmt.Errorf("safe-outputs.approve-workflow-run.allowed-workflows: invalid wildcard pattern %q: %w", pattern, err)
}The exact helper can vary, but the important part is: validate the pattern itself, not whether it happens to match the empty string.
|
@copilot run pr-finisher skill |
Completed the pr-finisher pass: local |
Triage
Substantial change (+1685 lines,
|
|
🎉 This pull request is included in a new release. Release: |
Adds an opt-in safe output that triggers GitHub’s “Approve to run” action for workflow runs blocked by fork pull request approval.
Safe output
approve-workflow-runconfiguration andapprove_workflow_runMCP tool.actions: write.Runtime safeguards
action_requiredconclusion.Developer surface
Caution
agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.
Details
Potential security threats were detected in the agent output.
Review the workflow run logs for details.
@copilotQuick triage for maintainer-ready follow-up:Please refresh the branch if GitHub allows it, address the remaining maintainer-facing follow-up, and run the
pr-finisherskill before handing this PR back to maintainers.Outstanding review items (newest first):
94f3339now rejectspull_request_targetand disables fork PR approvals by default unlessfork: true; please reply in-thread summarizing the final behavior, confirm the workflow-run approval gate now keys off the real pending state, and resolve any stale threads that are already answered.approve_workflow_runis present and no malformed payload bypass remains.approve_workflow_runis now in validation config with the requiredrun_idcontract and coverage.Failed checks from the compact candidate set:
Branch update was requested automatically for this run when GitHub allows it.
Run context: https://github.com/github/gh-aw/actions/runs/31901511033> Generated by 👨🍳 PR Sous Chef · gpt54 · 7.83 AIC · ⌖ 8.41 AIC · ⊞ 8.7K · ◷
Run context: https://github.com/github/gh-aw/actions/runs/31902813236> Generated by 👨🍳 PR Sous Chef · gpt54 · 6.1 AIC · ⌖ 7.25 AIC · ⊞ 8.7K · ◷
Run context: https://github.com/github/gh-aw/actions/runs/31913499533> Generated by 👨🍳 PR Sous Chef · gpt54 · 5.24 AIC · ⌖ 5.71 AIC · ⊞ 6.3K · ◷