Treat non-waiting workflow runs as skipped in approve_workflow_run#54895
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot approve workflow run only applies to "action_required" state |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Good catch — the handler was gating on Fixed in a0d1b9d with an |
|
@copilot this is not just for forks, it is also happening for agent-initiated PRs like copilot/* PRs. Check current PRs and review workflow runs. You should find runs n the action required state. Extend the safe output to only consider PRs from this repo by default. Workflows from forks must be explicitely allowed in allowed-repos list. (Defaults to current repo) |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Confirmed from the Actions API: runs in the action-required state include same-repo agent PRs (e.g. Replaced the boolean
|
|
@copilot resolve the merge conflicts on this branch. |
|
@pelikhan Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub. You can ask me to try again later by mentioning me in a new comment. If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: Sorry for the inconvenience! |
There was a problem hiding this comment.
Pull request overview
Makes workflow-run approval races non-fatal while introducing repository-scoped fork authorization.
Changes:
- Classifies non-awaiting runs and max-limit cases as skipped.
- Replaces
forkwithallowed-repos. - Updates tests, schemas, documentation, and generated workflow output.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_outputs_handler_registry.go |
Emits allowed repositories. |
pkg/workflow/js/safe_outputs_tools.json |
Updates tool description. |
pkg/workflow/approve_workflow_run.go |
Parses allowed repositories. |
pkg/workflow/approve_workflow_run_test.go |
Updates parser tests. |
pkg/workflow/approve_workflow_run_integration_test.go |
Verifies generated configuration. |
pkg/parser/schemas/main_workflow_schema.json |
Replaces the fork schema option. |
docs/src/content/docs/specs/safe-outputs-specification.md |
Updates approval requirements. |
docs/src/content/docs/reference/safe-outputs-pull-requests.md |
Documents new behavior. |
docs/src/content/docs/reference/glossary.md |
Updates the glossary entry. |
docs/src/content/docs/reference/frontmatter-full.md |
Updates frontmatter reference. |
actions/setup/js/safe_outputs_tools.json |
Updates runtime tool description. |
actions/setup/js/approve_workflow_run.test.cjs |
Covers states and repository allowlists. |
actions/setup/js/approve_workflow_run.cjs |
Implements skipping and repository checks. |
.github/workflows/pr-sous-chef.md |
Removes obsolete fork configuration. |
.github/workflows/pr-sous-chef.lock.yml |
Regenerates compiled workflow output. |
.github/skills/agentic-workflows/SKILL.md |
Removes an unrelated routing entry. |
.github/aw/safe-outputs-automation.md |
Updates automation guidance. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 17/17 changed files
- Comments generated: 3
- Review effort level: Balanced
| if (!isAwaitingApproval(run)) { | ||
| // Benign race: by the time the safe_outputs job runs, the workflow run may have | ||
| // already been approved (by a human or an earlier run) and moved past the | ||
| // pending-approval state. There is nothing left to do, so report this as a skipped | ||
| // no-op instead of a failure that would fail the whole safe outputs step. | ||
| const reason = `Workflow run ${runId} is not awaiting approval (status: ${run.status || "none"})`; | ||
| core.warning(reason); | ||
| return { success: false, skipped: true, reasonCode: "NOT_AWAITING_APPROVAL", reason, error: reason }; |
| "allowed-repos": { | ||
| "type": "array", | ||
| "items": { | ||
| "type": "string", | ||
| "minLength": 1 | ||
| }, | ||
| "description": "Repositories in format 'owner/repo' whose pull requests may have their workflow runs approved, in addition to the current repository which is always allowed. Use this to allow fork pull requests. Supports wildcard patterns such as 'org/*'." |
| @@ -103,6 +103,5 @@ After loading the matching workflow prompt or skill, follow it directly: | |||
| - Choose workflow architecture and patterns: `.github/aw/patterns.md` | |||
| - Optimize token usage and cost: `.github/aw/token-optimization.md` | |||
| - Design long-running multi-agent research workflows: `.github/aw/multi-agent-research.md` | |||
…78-939d-b60348abbd40
PR Sous Chef's
Process Safe Outputsstep has been failing ~8% of runs, generating a steady stream of duplicate[aw] Failed jobsissues. The suspected cause was a whole-batch validation abort, but the logs from run 32543132845 show something narrower.Root cause
The per-item loop in
safe_output_handler_manager.cjsalready degrades gracefully — every item is processed, which is whyUpload Safe Outputs Itemsstill succeeds. The bug is item classification, not batch abort.approve_workflow_runreturned a fatal failure whenever the target run was no longer inwaiting. On a 15-minute cadence this is a routine race: by the time thesafe_outputsjob runs, the run has usually already been approved by a human or by an earlier PR Sous Chef run. Nothing is left to do, yet the hard failure fedcore.setFailedand failed the step.Changes
actions/setup/js/approve_workflow_run.cjsrun.status !== "waiting"now returns a skipped result so the manager's existingresult.skipped === truebranch reports it in the summary without failing the step.add_comment,create_issue):skipped: true+reasonCode: "MAX_COUNT_REACHED".actions/setup/js/approve_workflow_run.test.cjs— assertions updated for both result shapes.Genuine authorization and configuration errors — fork without
fork: true, protected file changes, disallowed workflow, unauthorized PR, API errors — remain fatal.