Make CodeQL PR comments actually useful#117
Merged
Conversation
Contributor
greenc-FNAL
commented
Nov 14, 2025
greenc-FNAL
commented
- Useful utility scripts for dealing with CodeQL alerts and report files
- PR comments now actually report something useful
Contributor
|
Review the full CodeQL report for details. |
knoepfel
reviewed
Nov 18, 2025
Member
knoepfel
left a comment
knoepfel
left a comment
There was a problem hiding this comment.
@greenc-FNAL, thanks for the PR. I have several questions/comments. I'm not sure that the resolution of any of them should hold up this PR.
It's unclear to me what level of rigor we should apply to code that has been generated by AI. I think we should at least be able to vet what the code is doing, but that can be difficult, and this PR is not an exception to that.
Contributor
Author
|
@jules please resolve the following error with the CodeQL CI check: The relevant agent task is https://jules.google.com/session/2491371005401655318/code/scripts/check_codeql_alerts.py |
- Provide token where needed
- Fix CI workflow errors
- Fix AI thinko
- Work around comment permissions issue
- Getting into the weeds
- Refactor CodeQL workflow to fix PR commenting
This change addresses the failure of the CodeQL workflow to comment on pull requests. The previous implementation was overly complex, with two separate workflows attempting to handle analysis and commenting, leading to permission errors.
- Deleted `.github/workflows/codeql-comment.yml`, a redundant workflow that was created as a workaround.
- Updated `.github/workflows/codeql-analysis.yaml` to grant it `pull-requests: write` permissions, allowing it to post comments directly on PRs.
- Removed the unnecessary artifact upload steps from the main workflow, as the comment is now posted directly.
This consolidates all CodeQL logic into a single, streamlined workflow that now has the correct permissions to function as intended.
- Refactor CodeQL debug log handling for clarity and reliability
This change streamlines the handling of the debug log in the CodeQL workflow.
- The `check_codeql_alerts.py` script now accepts a `--log-path` argument, removing the hardcoded log file path.
- The `codeql-analysis.yaml` workflow now defines the log path in a single environment variable and passes it to the script.
- The redundant "fallback" log upload step has been removed, and the primary upload step has been renamed for clarity.
This makes the debug log handling more robust, maintainable, and easier to understand.
- Refine CodeQL script logic and PR comment content
- Refactor CodeQL script for clarity and maintainability:
- Consolidated duplicated summary-printing logic into a single `_print_summary` function.
- This improves maintainability and ensures consistent output.
- Refine PR comment to focus on PR-specific changes:
- Removed the comparison against the repository's 'main' branch from the PR comment.
- The comment now only shows alerts that are new or fixed relative to the PR's branch point and its previous commit.
- This makes the comment more concise and directly relevant to the changes in the pull request.
- Fix CodeQL workflow validation error
This change corrects a workflow syntax error caused by referencing the `runner` context in a job-level `env` block.
- The invalid `CODEQL_ALERTS_LOG_PATH` environment variable has been removed from the `codeql-report` job.
- A new initial step, "Set log file path," has been added to the `codeql-report` job. This step correctly defines the log file path using `$RUNNER_TEMP` and makes it available to subsequent steps via a step output.
- The script invocation and artifact upload steps have been updated to reference this new step output.
This resolves the "Unrecognized named-value: 'runner'" error and ensures the workflow is valid.
This change adjusts the format of the CodeQL PR comment to be more concise and directly relevant to the pull request's lifecycle. - The "Alert Matching Summary," which compared the PR to the `main` branch, has been replaced with a more focused summary. - The new summary highlights the number of new and fixed alerts relative to the PR's **branch point** and its **previous commit**. - This provides developers with a clearer, more actionable summary of how the PR has impacted the CodeQL alerts within its own context. - The detailed lists of new and fixed alerts for these comparisons are preserved.
Co-authored-by: Kyle Knoepfel <knoepfel@fnal.gov>
- Replace the `in locals()` check for variable existence with a more explicit pattern of initializing variables to `None` and then checking `is not None`.
Refactor the `check_codeql_alerts.py` script to improve its structure and remove all uses of `locals()`. - The large `main()` function has been broken down into smaller, more focused helper functions (`_compare_alerts_via_api` and `_build_multi_section_comment`). - A new `APIAlertComparison` dataclass is used to provide a clear data structure for API comparison results. - This refactoring eliminates the need for `locals()` by ensuring variables are properly initialized and passed between functions.
Refactor the `check_codeql_alerts.py` script to improve its structure and remove all uses of `locals()`. - The large `main()` function has been broken down into smaller, more focused helper functions (`_compare_alerts_via_api` and `_build_multi_section_comment`). - A new `APIAlertComparison` dataclass is used to provide a clear data structure for API comparison results. - This refactoring eliminates the need for `locals()` by ensuring variables are properly initialized and passed between functions. Fixes the following bugs introduced during the refactoring: - A `NameError` caused by incorrect class definition order. - A regression where matched alerts were no longer reported in the summary output. - A regression in the logic for when to post a PR comment.
greenc-FNAL
force-pushed
the
maintenance/codeql-fix-pr-comment
branch
from
e46e6ca to
0bc5cad
Compare
knoepfel
approved these changes
Nov 19, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.