perf(ci): scope semgrep-general to ERROR-severity rules - #77647
Conversation
PROPOSAL — needs a security-team call before merging. p/trailofbits accounts for 11.0 of semgrep-general's 12.5 min, and its cost sits in the non-ERROR rules. Measured on CI: all three heavy packs with --severity=ERROR run in 1.4 min. The tradeoff is real. This drops 474 rules that apply to the languages this job scans, all WARNING/INFO/MEDIUM, and keeps all 189 ERROR ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
fbe5137 to
b4fd4bb
Compare
|
Reviews (1): Last reviewed commit: "perf(ci): scope semgrep-general to ERROR..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
This PR adjusts the semgrep-general CI job to run only ERROR-severity Semgrep rules, aligning it with the existing “ERROR-fails” model used elsewhere in the workflow (e.g., semgrep-desktop) and significantly reducing runtime at the cost of dropping WARNING/INFO/MEDIUM coverage for the paths uniquely covered by semgrep-general.
Changes:
- Add
--severity=ERRORto thesemgrep-generalSemgrep invocation to filter the job to ERROR-severity rules/findings only.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Warning
Proposal, not a fix. Do not merge without a security-team call.
This trades SAST coverage for CI time. I have measured both sides so the tradeoff is concrete, but the decision is not mine. Independent of #77481, which stops the bleeding without touching coverage and lands on its own; either can merge first.
Problem
semgrep-generaltakes ~12 minutes. #77481 traced that to a semgrep regression (1.163.0 → 1.167.0, #65921) and stopped it from failing PRs, but did not make it fast again. Two rounds of CI isolation jobs pinned the cost:p/trailofbitsalonep/owasp-top-tenalonep/security-auditalone--severity=ERRORThe cost is not the pack itself so much as its non-ERROR rules, which is where the generic
<multilang>rules live.Changes
One line:
--exclude-rule yaml.github-actions.security.audit.unsafe-add-mask-workflow-command.unsafe-add-mask-workflow-command \ + --severity=ERROR \ --error \semgrep-desktopalready does exactly this, which is why it scans 4,827 files in 1 minute with these same packs.The tradeoff, measured
Of the 812 rules loaded, 663 apply to the languages this job actually scans. Against that set:
474 rules stop running on this job's file set. Every ERROR-severity rule is kept.
Two things worth weighing:
This job is the only place many of those rules run. 494 of the dropped rules come from
p/owasp-top-ten/p/security-audit, whichsemgrep-pythonandsemgrep-jsalso load without a severity filter. But those jobs scanposthog/,products/,frontend/,nodejs/, andservices/— precisely the pathssemgrep-generalexcludes. So forpackages/,tools/,bin/,.github/,terraform/, and friends, this is the only coverage there is.We are already doing this one rule at a time. All four existing
--exclude-ruleentries on this job are WARNING severity, so--severity=ERRORmakes every one of them redundant. The team has been suppressing WARNING noise individually; this is the same decision made wholesale.Sample of what goes away, to make it concrete rather than a number:
The alternative, for comparison
Dropping
p/trailofbitsfrom this job instead:--severity=ERROR(this PR)p/trailofbitsNarrower blast radius, but it gives up 29 ERROR-severity rules, and this PR gives up none. Which matters more is the judgment call. There is also a third option that costs no coverage at all: report the amd64 regression upstream to semgrep and wait.
How did you test this code?
Not yet run in CI on this branch. The 1.4 min figure comes from the
diag-severity-errorjob on #77481, which ran these three packs with--severity=ERRORagainst the same file set on the same runner type, alongside the unmodified 12.5 min scan as a same-commit control.Rule counts were computed by fetching the four packs from the semgrep registry, deduplicating by rule ID (812 unique), and bucketing by
severityandlanguages, filtered to the languages this job's scan-status table reports.bin/hogli lint:workflows- 7/7 pass.I have not verified that zero findings appear, since the point of the change is that fewer rules run. If this direction is accepted, the merge-blocking question is coverage, not correctness.
Automatic notifications
Docs update
None needed.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Claude (Claude Code, Opus 5) did the investigation and drafted this.
This exists to give the security team something concrete to react to rather than an abstract question. The measurement work is in #77481; this PR is just the one-line change plus the coverage accounting.
The honest framing: I can tell you what it costs and what it saves, but not whether 474 WARNING-severity rules over
packages/,tools/,bin/and.github/are worth 11 minutes of CI on ~8% of PRs. After #77481's path gate this job rarely runs, so the pressure to accept the tradeoff is a lot lower than it looks. Doing nothing is a reasonable outcome.