maintenance: correct tidy-fix comment reactions, and aggressive consolidation#411
maintenance: correct tidy-fix comment reactions, and aggressive consolidation#411greenc-FNAL wants to merge 10 commits intomainfrom
tidy-fix comment reactions, and aggressive consolidation#411Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Pull request overview
This PR consolidates PR-comment reaction handling for @phlexbot ...-fix workflows by introducing a reusable composite action that removes the initial “eyes” reaction and adds a completion reaction based on workflow outcome.
Changes:
- Added a new composite action (
complete-pr-comment) to centralize reaction cleanup + completion reactions. - Updated multiple fix workflows to call the new action (and to run it under
always()so it executes on failures). - Added a completion job to
clang-tidy-fixto ensure reactions are updated after tidy + PR-comment steps.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/yaml-fix.yaml | Replaces inline reaction github-script steps with complete-pr-comment. |
| .github/workflows/python-fix.yaml | Replaces inline reaction github-script steps with complete-pr-comment. |
| .github/workflows/markdown-fix.yaml | Replaces inline reaction github-script steps with complete-pr-comment. |
| .github/workflows/jsonnet-format-fix.yaml | Replaces inline reaction github-script steps with complete-pr-comment. |
| .github/workflows/header-guards-fix.yaml | Replaces inline reaction github-script steps with complete-pr-comment. |
| .github/workflows/cmake-format-fix.yaml | Replaces inline reaction github-script steps with complete-pr-comment. |
| .github/workflows/clang-format-fix.yaml | Replaces inline reaction github-script steps with complete-pr-comment. |
| .github/workflows/format-all.yaml | Uses complete-pr-comment with an aggregated status across sub-workflows. |
| .github/workflows/clang-tidy-fix.yaml | Adds a new job to update reactions for clang-tidy runs. |
| .github/actions/complete-pr-comment/action.yaml | New composite action implementing reaction removal + success/failure reaction. |
Comments suppressed due to low confidence (1)
.github/actions/complete-pr-comment/action.yaml:53
- This file appears to end with an extra blank line (trailing blank line at EOF). The repo’s formatting rules require files to end with exactly one newline and no trailing blank lines; please remove the final empty line.
}
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 8 comments.
You can also share your feedback on Copilot code review. Take the survey.
|
@phlexbot format |
|
@phlexbot yaml-fix |
|
Automatic YAML formatter fixes pushed (commit 4b59517). |
4b59517 to
6aca4df
Compare
tidy-fix comment reactions, and aggressive consolidation
This commit introduces a new reusable GitHub Action `.github/actions/complete-pr-comment`
to handle removing the 'eyes' reaction and adding a completion reaction ('rocket' for success,
'confused' for failure) to triggering issue comments.
Changes:
- Created `.github/actions/complete-pr-comment/action.yaml` with robust error handling.
- Updated `clang-tidy-fix.yaml` to include reaction handling (fixing the reported issue).
- Refactored all other format fix workflows to use the new consolidated action.
- Improved `format-all.yaml` to report the aggregate status of all formatting sub-jobs.
Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
maintenance: Consolidate workflow reactions and setup logic
This commit introduces a major refactoring of the CI workflows to reduce
duplication, improve maintainability, and ensure consistent PR comment
reactions.
New Reusable Actions:
- .github/actions/complete-pr-comment: Robustly handles PR comment
reactions (removing 'eyes', adding 'rocket' on success or 'confused'
on failure) with proper error handling.
- .github/actions/prepare-fix-outputs: Consolidates the resolution of
branch and repository information for workflows triggered by bot commands.
- .github/actions/prepare-check-outputs: Consolidates environment
detection and ref resolution for check-style workflows.
Improvements:
- Fixed missing completion reactions in clang-tidy-fix.
- Refactored all 21 core workflow files to use the new actions,
significantly reducing duplicated boilerplate.
- Preserved all original authorization triggers and permissions.
- Enhanced format-all.yaml to report an aggregate status.
- Consistently used @main for internal action references as requested.
Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
maintenance: Fix and consolidate PR comment reactions and workflow setup
This commit addresses the missing 'rocket' emoji on completion of fix workflows
and performs a major refactoring of the CI workflows to reduce duplication
and improve maintainability.
Key Changes:
- Created `.github/actions/complete-pr-comment`: Robustly handles PR comment
reactions (removing 'eyes', adding 'rocket' on success or 'confused'
on failure) with proper error handling.
- Created `.github/actions/prepare-fix-outputs`: Consolidates ref and
repository resolution for bot-triggered workflows.
- Created `.github/actions/prepare-check-outputs`: Consolidates environment
detection and ref resolution for check workflows.
- Refactored all 21 core workflow files to use these new actions.
- Fixed specific issues in `clang-tidy-fix.yaml`, `codeql-analysis.yaml`,
and `format-all.yaml` identified during review.
- Removed temporary refactoring scripts.
These changes ensure consistent behavior and significantly reduce YAML boilerplate.
Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
Apply YAML formatter fixes
6aca4df to
8fd1ce2
Compare
| - name: Check out source code | ||
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 | ||
| with: | ||
| fetch-depth: 0 | ||
| path: ${{ inputs.checkout-path }} | ||
| ref: ${{ inputs.ref }} | ||
| repository: ${{ inputs.repo }} | ||
|
|
||
| - name: Detect relevant changes |
Check warning
Code scanning / CodeQL
Checkout of untrusted code in trusted context
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 9 hours ago
In general, to fix this class of problem you must ensure that untrusted pull-request code is not checked out (or, if it must be, not executed) in a privileged context. For a reusable composite action, the safest approach is to (a) constrain the repository and ref that can be checked out so that they are not directly controllable by PR authors, or (b) clearly separate “trusted” and “untrusted” modes, requiring an explicit, safe ref when used in privileged workflows.
For this specific file, the minimal change that hardens the action without altering intended functionality is to restrict repository to the current repository and eliminate the ability for callers to override repo arbitrarily. That removes a major injection vector where a privileged workflow could be tricked into checking out an attacker-controlled repository. Additionally, we can narrow the ref to only allow values that are expected to be trusted (for example defaulting to the workflow’s own ref) or at least document that ref must not be wired directly from untrusted PR HEAD in privileged workflows; however, we are constrained to code-only changes and cannot enforce patterns at the call site. Within this file, the best concrete hardening we can do is:
- Remove
repoas a configurable input for checkout and instead userepository: ${{ github.repository }}so the checkout is always from the current repo. - Optionally, we can leave the
repoinput defined for backward compatibility for callers, but it will no longer affect the checkout. This avoids breaking existing workflows while still preventing unexpected external repos from being checked out in a privileged job.
These edits are all within .github/actions/run-change-detection/action.yaml in the Check out source code step (lines 40–47). No new imports or external dependencies are needed.
| @@ -43,7 +43,8 @@ | ||
| fetch-depth: 0 | ||
| path: ${{ inputs.checkout-path }} | ||
| ref: ${{ inputs.ref }} | ||
| repository: ${{ inputs.repo }} | ||
| # Always check out from the current repository to avoid untrusted external repos | ||
| repository: ${{ github.repository }} | ||
|
|
||
| - name: Detect relevant changes | ||
| id: filter | ||
| @@ -65,4 +66,3 @@ | ||
| echo "::group::Relevant files detected" | ||
| printf '%s\n' "${{ steps.filter.outputs.matched_files }}" | ||
| echo "::endgroup::" | ||
| fi |
|
Review the full CodeQL report for details. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 20 comments.
Comments suppressed due to low confidence (1)
.github/workflows/jsonnet-format-fix.yaml:112
- The Jsonnet formatting step still uses
CHECKOUT_PATH: ${{ env.local_checkout_path }}, but checkout now happens at${{ needs.setup.outputs.checkout_path }}. These can diverge wheninputs.repopoints at a different repo name thangithub.event.repository.name(or whenworkflow-setupresolves a different default), causingfind "$CHECKOUT_PATH" ...to operate on a non-existent directory and apply no fixes. Use${{ needs.setup.outputs.checkout_path }}consistently forCHECKOUT_PATH.
- name: Apply Jsonnet formatting
id: lint
env:
CHECKOUT_PATH: ${{ env.local_checkout_path }}
# yamllint disable rule:line-length
run: |
docker run --rm -v "${{ github.workspace }}:/work" -w /work --user root public.ecr.aws/bitnami/jsonnet:latest \
sh -c "find \"$CHECKOUT_PATH\" \( -name '*.jsonnet' -o -name '*.libsonnet' \) -print0 | xargs -0 -r jsonnetfmt -i"
You can also share your feedback on Copilot code review. Take the survey.
|
@copilot Please address all outstanding current and historical review comments in the following ways:
|
|
@greenc-FNAL I've opened a new pull request, #414, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 5 comments.
You can also share your feedback on Copilot code review. Take the survey.
|
@copilot Please address outstanding, unresolved review comments from the latest review. |
|
@greenc-FNAL I've opened a new pull request, #419, to work on those changes. Once the pull request is ready, I'll request review from you. |
…sion (#419) * Initial plan * Fix artifact nested path and add issues:write permission for reactions Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@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>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 19 comments.
You can also share your feedback on Copilot code review. Take the survey.
|
@copilot Please address current unresolved review comments, excepting any that refer to changing the branch/version/hash reference to any reusable actions. Pay special attention to careful analysis and resolution of possible workflow permission issues. |
|
@greenc-FNAL I've opened a new pull request, #420, to work on those changes. Once the pull request is ready, I'll request review from you. |
* Initial plan * fix: address unresolved review comments - permissions, workflow_dispatch, pr-head-sha wiring Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@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>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 8 comments.
You can also share your feedback on Copilot code review. Take the survey.
|
@copilot Please address unresolved comments from the latest review, but more generally I think we need to ensure that—for all workflows—relevance detection either runs as expected or is bypassed according to expectations from It is of paramount importance that all workflow/action functionality from |
|
@greenc-FNAL I've opened a new pull request, #421, to work on those changes. Once the pull request is ready, I'll request review from you. |
…d correct clang-tidy/format-all failure behavior (#421) * Initial plan * fix: wire workflow_call inputs, act fallback for change detection, and clang-tidy/format-all failures Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@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>
|
@phlexbot yaml-fix |
|
Automatic YAML formatter fixes pushed (commit 333f6d3). |
|
@phlexbot markdown-fix |
|
No automatic markdownlint fixes were necessary. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 20 comments.
You can also share your feedback on Copilot code review. Take the survey.
f333886 to
749ae5b
Compare
|
@knoepfel This was tested as much as possible with a temporary change to reusable action locations, which has been reverted for (hopefully) final review and merge. |
clang-tidy-fixworkflowPR created automatically by Jules for task 18132869807280577001 started by @greenc-FNAL