docs: correct the stale --error comment in the semgrep job - #9
Merged
Conversation
The job comment still claimed "semgrep ALWAYS runs with --error, so any finding turns this job red". That flag was removed when the SARIF reporter took over the exit decision, so the comment described the opposite of the behaviour and implied the WARNING rules gate builds. Replaced with the two gates spelled out separately, since conflating them is the easy mistake: bin/report-sarif.sh decides WHICH findings turn the job red (error level only, suppressed excluded), and continue-on-error decides whether that red BLOCKS the caller. Also notes that semgrep's own "Blocking" label in its console summary is its policy tag and affects neither. Adds a regression guard. Re-adding --error would make semgrep exit non-zero on any finding, and under set -e that kills the step before report-sarif.sh runs — silently making the WARNING rules blocking and deleting the findings table. bin/test-report-sarif.sh now asserts the flag is absent and the reporter is called; mutation-checked by re-adding the flag and watching it fail. Swept the rest: the three WARNING rules describe themselves accurately, and the remaining --error mentions are this explanation and an accurate historical note.
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
1 participant
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.
Follow-up to review feedback that a comment had gone stale. It had — the
opposite file, and the opposite direction from the one flagged.
cc-generic-catch.yaml's comment is accurate.--errorwas removed in #5when
report-sarif.shtook over the exit decision, and that script counts onlyerror-level results. WARNING rules genuinely do not gate the build.
security.ymlwas the stale one, still asserting the removed flag:That described the opposite of the behaviour and implied the WARNING rules block.
Replaced with the two gates stated separately, since conflating them is the easy
mistake:
bin/report-sarif.sh, error-level only,suppressed excluded.
continue-on-error/semgrep-blocking.Plus a note that semgrep's own "Blocking" label in its console summary is its
policy tag and affects neither.
Regression guard
Re-adding
--erroris a one-word change with two silent consequences: semgrepexits non-zero on any finding, and under
set -ethat kills the step beforereport-sarif.shruns — so the WARNING rules start blocking and the findingstable disappears.
bin/test-report-sarif.shnow asserts the flag is absent and the reporter isinvoked. Mutation-checked by re-adding the flag and confirming the test fails.
Swept the rest of the repo: all three WARNING rules describe themselves
accurately, and the only remaining
--errormentions are this explanation and anaccurate historical note.
🤖 Generated with Claude Code