Fix ref input, eyes reaction handling, and docs consistency in fix workflows#385
Conversation
…ssage Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR tightens up reusable “fix” workflow documentation/inputs so callers pass a branch ref (not a SHA), and hardens format-all.yaml’s reaction cleanup and patch-artifact messaging.
Changes:
- Updated
workflow_callrefinput descriptions across fix workflows to require a branch name (not a commit SHA). - Updated reusable workflow documentation examples and input docs to use
pull_request.head.refand describe the branch-name requirement. - Refactored
format-all.yamlto safely find-and-delete the “eyes” reaction and clarified the patch artifact message.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/yaml-fix.yaml | Clarifies workflow_call.inputs.ref as a branch name. |
| .github/workflows/python-fix.yaml | Clarifies workflow_call.inputs.ref as a branch name. |
| .github/workflows/markdown-fix.yaml | Clarifies workflow_call.inputs.ref as a branch name. |
| .github/workflows/jsonnet-format-fix.yaml | Clarifies workflow_call.inputs.ref as a branch name. |
| .github/workflows/header-guards-fix.yaml | Clarifies workflow_call.inputs.ref as a branch name. |
| .github/workflows/cmake-format-fix.yaml | Clarifies workflow_call.inputs.ref as a branch name. |
| .github/workflows/clang-format-fix.yaml | Clarifies workflow_call.inputs.ref as a branch name. |
| .github/workflows/format-all.yaml | Makes eyes-reaction deletion conditional and improves patch messaging. |
| .github/REUSABLE_WORKFLOWS.md | Updates examples/inputs to use head.ref and document branch-name ref. |
.github/workflows/format-all.yaml
Outdated
| const { data: reactions } = await github.rest.reactions.listForIssueComment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| comment_id: context.payload.comment.id, | ||
| reaction_id: (await github.rest.reactions.listForIssueComment({ | ||
| comment_id: context.payload.comment.id | ||
| }); |
There was a problem hiding this comment.
The listForIssueComment call is not guarded. If the API call fails (permissions/transient errors), this step will throw and can fail combine-results, preventing the combined PR comment from being posted. Since reaction cleanup is best-effort, wrap the list+delete logic in a try/catch (or mark the step continue-on-error: true) so the workflow can continue even if reactions can’t be queried.
915029e
into
maintenance/phlexbot-format-rollup
* Combine reports from all `@phlexbot format` workflows - Dedicated new workflow `format-all.yaml` is the sole responder to `@phlexbot format` instead of multiple "fix" workflows. - Specific "fix" workflows are called via `workflow_call` and results combined into a single comment to reduce clutter in the PR conversation history. - The `handle-fix-commit` reusable action will skip posting generated comments when called via `workflow_call`, allowing multiple messages to be consolidated into a single PR comment. - Indicate workflow completion on invocation comment * Update workflow documentation * Prettier fixes * Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Apply review feedback: string skip-comment, fix comparisons, header-guards run script, failure reporting Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com> * Run results collection step with Bash - Per #383 (comment) * fix: guard inputs.skip-comment in issue_comment workflows and update docs - Replace `inputs.skip-comment` with `github.event_name == 'workflow_call' && inputs.skip-comment || 'false'` in all 7 fix workflows so that issue_comment triggers don't access the unavailable inputs context - Remove redundant `&& inputs.skip-comment != 'true'` from reaction step conditions that already gate on `github.event_name == 'issue_comment'` - Update format-all.yaml patch message to include artifact name and apply instructions - Update REUSABLE_WORKFLOWS.md: change skip-comment examples from boolean true to string "true", and type descriptions from boolean to string Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Fix ref descriptions, docs examples, eyes reaction, patch artifact message Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com> * Address remaining Copilot review comments - Per #385 (review) * Address latest review - Per #385 (review) * Harden Remove eyes reaction step in all fix workflows Wrap listForIssueComment call in try/catch so a transient API error does not fail the workflow. Narrow the bot-match predicate from the broad r.user.type === 'Bot' to r.user && r.user.login === 'github-actions[bot]' so only the reaction posted by phlexbot's pre-check step is removed. Use destructured { data: reactions } assignment for consistency with format-all.yaml. * Declare on.workflow_call.outputs in all fix workflows format-all.yaml reads changes/pushed/commit_sha_short/patch_name from needs.<job>.outputs.* for each called fix workflow. GitHub Actions only surfaces these to the caller when they are explicitly re-exported under on.workflow_call.outputs; job-level outputs alone are not visible to calling workflows. Add on.workflow_call.outputs sections to all seven fix workflows, mapping to the corresponding apply-job outputs. * Guard PR lookup in handle-fix-commit against missing issue context The Get PR maintainer_can_modify property step uses context.issue.number, which is only populated when the root trigger is an issue_comment event. When a fix workflow is triggered via workflow_dispatch, context.issue.number is undefined, causing the pulls.get API call to fail whenever formatting changes are found. Note: workflow_call from format-all.yaml (triggered by issue_comment) is not affected — GitHub propagates the event payload to called workflows so context.issue.number is populated there. The bug is specific to the workflow_dispatch path. Default to 'false' for maintainer_can_modify when no PR context is available. The commit path's first condition (same-repo check) still allows pushes to proceed correctly for workflow_dispatch runs. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
Review feedback on the format-all consolidation PR identified several issues: fix workflows accepting commit SHAs as
ref(which breaksgit push origin HEAD:<ref>), docs examples usinghead.shainstead ofhead.ref, a fragile eyes-reaction removal that could passundefinedasreaction_id, and a misleading patch artifact message.Fix workflow
refinputUpdated
workflow_callrefdescription in all 7 fix workflows to make the branch-name requirement explicit:Documentation
head.shatohead.refAll Inputsrefentries updated to reflect the branch-name requirementRemove eyes reactioninformat-all.yamlReplaced the inline chained call (which passed
undefinedtodeleteForIssueCommentwhen no eyes reaction existed) with an explicit fetch-then-conditionally-delete pattern:Patch artifact message
Clarified that the patch file (
fix.patch) lives inside thefix-patchartifact, not that the artifact is named after the patch file.💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.