fix(ci): stop semgrep-general timing out on every PR - #78225
Merged
Conversation
semgrep-general was the only semgrep job without a changed-paths gate, so it ran on 100% of PRs while scanning only the directories the other jobs exclude. Its p50 is 12.3 min against a 20 min timeout, so slow runners tripped it and blocked the required Semgrep Checks Pass check. Gate it on the complement of its own --exclude list and raise the timeout to 30, matching semgrep-python. Coverage is unchanged: master pushes and oversized PRs still bypass the filter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
😎 Merged successfully - details. |
1 task
Contributor
|
Reviews (1): Last reviewed commit: "fix(ci): stop semgrep-general timing out..." | Re-trigger Greptile |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces merge-queue friction from semgrep-general by making it run only when its scan targets could have changed, and by increasing its timeout to avoid flaky cancellations from runner variance. It updates the Security workflow so required Semgrep checks still fail closed while allowing legitimate skips.
Changes:
- Add a
generalpath filter output to the existingchangesjob, covering the complement ofsemgrep-general’s--excludelist. - Gate
semgrep-generalonneeds: changes+needs.changes.outputs.general == 'true'so it skips on most PRs. - Increase
semgrep-generaltimeout-minutesfrom 20 → 30 (matching the existing pattern used for other heavier semgrep shards).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
feliperalmeida
approved these changes
Aug 5, 2026
rnegron
approved these changes
Aug 5, 2026
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
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.
Note
This recreates #77481 on a fresh branch, after that one got stuck in the merge queue. The diff and the analysis are unchanged. #77481 carries the review history and the diagnostic CI runs cited below.
Problem
semgrep-generaltrips its own timeout often enough to be a real tax on the merge queue. When it does, GitHub reports the job ascancelledrather thanfailed, so it doesn't read like a normal CI failure. Example run: the job ran 20m15s against a 20 min limit and was killed mid-scan, while every other job in that workflow passed.I sampled every
Securityrun from Jul 28 to Aug 4 (692 completedsemgrep-generaljobs):1.4% overall, and in one 2.5 hour window on Aug 4 it was 7 of 258 (2.7%).
Semgrep Checks Passis a required check, so each one blocks a PR or kicks it out of the queue.Root cause: the job got 4x slower on Jun 24
This job used to take 3 minutes. Sampling its duration back through the year:
Not gradual growth from the repo getting bigger. A step change overnight. Pulling all 598
semgrep-generalruns from Jun 24 puts the cutover inside a two minute window:#65921 (
fix(ci): upgrade semgrep 1.163.0 → 1.167.0 to fix intermittent hang) merged at 20:54:35 UTC, eight seconds before the first slow run started. It is the only change toci-security.yamlor.semgrep/that day. Before it, 455 runs at p50 3.1 min. After it, 143 runs at p50 12 min.Warning
The 1.167.0 pin is the actual cause of the slowness, and it is still in place. This PR does not fix that, it just stops it from failing PRs. I tried bumping the version and it did not help, see below.
That regression left a 12 min p50 under a 20 min timeout, about 1.6x headroom, so ordinary runner variance tips it over. #70907 added
--jobs 4three weeks ago to claw the time back and the tail did not move.Second, independent problem:
semgrep-generalwas the only semgrep job without aneeds: changesgate. It scans 1,605 files inpackages/,tools/,.github/,bin/and friends, since everything else is--excluded. Of the last 400 first-parent merges to master, 370 (92.5%) touched nothing it scans and still paid for a 12 min job.Changes
Gate the job on the complement of its own
--excludelist, and raise the timeout to 30 to matchsemgrep-python.Note
Coverage is unchanged. Pushes to master skip the filter entirely (
if: github.event_name != 'push'), so master still runs the full scan, and oversized PRs still force every scan viaforce_all. The aggregator already toleratedskippedfor this job, so no change was needed there.flowchart TD A{{PR}} --> B[changes path filter] B --> C[semgrep-python] B --> D[semgrep-js] B --> E[semgrep-devex] A --> F[semgrep-general<br/>always runs, 12 min] C --> G[Semgrep Checks Pass] D --> G E --> G F --> G classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; classDef phRed fill:#f54e00,stroke:#f54e00,color:#fff; classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000; class A,G phYellow; class B phBlue; class F phRed;flowchart TD A{{PR}} --> B[changes path filter] B --> C[semgrep-python] B --> D[semgrep-js] B --> E[semgrep-devex] B --> F[semgrep-general<br/>skipped on ~92% of PRs] C --> G[Semgrep Checks Pass] D --> G E --> G F --> G classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000; class A,G phYellow; class B,F phBlue;What I tried and rejected: bumping the version
I pushed 1.172.0 to #77481 to see if a newer build cleared the amd64 regression. It did not:
Nowhere near the 3.1 min it ran at before #65921, and if anything worse. I reverted it, so the pin stays at 1.167.0. One sample against a p50 on shared runners, so I would not claim 1.172.0 is worse, only that it plainly does not fix this.
The regression is isolated to one job
Splitting all 597 Jun 24 semgrep jobs on the 20:54:35 boundary:
So it is not a general engine slowdown. Whatever 1.167.0 changed, it only bites
semgrep-general's configuration. Two things are unique to that job: it is the only one using--config p/github-actions, and it is the only one running the full rule set without--severity=ERROR(which is what keepssemgrep-desktopat 1 min despite scanning 4,827 files with the same expensive packs, since it cuts 812 rules to 228 and the generic<multilang>rules from 24 to 3).Which pack: measured, not guessed
I ran two rounds of temporary diagnostic jobs on #77481 that split the rule packs, with the 4-pack
semgrep-generalas a same-commit control. My first guess wasp/github-actions, since it is unique to this job and itscurl-eval/gha-curl-pipe-shellrules throw parse errors 24 times per scan. That was wrong.Round 1 cleared it:
diag-only-github-actionsdiag-without-github-actionssemgrep-general(control)Round 2 pinned it:
diag-trailofbitsdiag-owaspdiag-security-auditdiag-severity-error--severity=ERRORsemgrep-general(control)So
p/trailofbitsis essentially the entire cost, 11.0 of 12.5 min, and--severity=ERRORacross all three packs collapses it to 1.4 min.This reconciles with everything else. Local profiling put trailofbits at 69s of 81s, the same shape. If trailofbits is ~11 of today's 12.5 min, it was ~2 of the pre-regression 3.1 min, so Jun 24 was a ~5.5x slowdown localized to that one pack rather than a broad engine change, which is why the other seven jobs stayed flat.
The diagnostic jobs have been removed. They were never wired into
semgrep_checks, so they gated nothing while they existed.Follow-ups (not in this PR)
Decide what to do about
p/trailofbits. Three options, all of them someone else's call since they trade off security coverage:--severity=ERRORtosemgrep-general, assemgrep-desktopalready does. Takes it to ~1.4 min. Costs real coverage: 812 rules drop to 228, and the generic<multilang>rules go from 24 to 3. Not a free win.p/trailofbitsin this job. Other jobs already load it, so the coverage loss is narrower than it looks.linux/amd64between 1.163.0 and 1.167.0, with no change on arm64, looks like a semgrep bug worth filing.curl-evalandgha-curl-pipe-shellare broken. Unrelated to performance, now thatp/github-actionsis cleared. They throwmetavariable-pattern failed when parsing24 times per scan against.github/workflows/**, so those two rules are not doing their job today.Priority note. With the path gate,
semgrep-generalruns on roughly 8% of PRs, so the 12 min is much less costly than it was. Worth fixing, no longer urgent.How did you test this code?
I could not reproduce the slowness locally, and chasing that is what found the root cause. On an arm64 Mac the exact CI command runs in 81 seconds against CI's ~12 minutes, so I A/B'd the pinned versions on identical hardware and flags:
Identical. The regression does not reproduce on arm64, which is why it points at the
linux/amd64build rather than at rule content. I confirmed both image digests are proper multi-arch OCI indexes with a reallinux/amd64entry, so it is not QEMU emulation. Both versions also emit the same 24 internal matching errors and 217 syntax errors, so they are doing the same work.Beyond the local A/B, I measured the pack isolation above in CI on #77481 across two rounds of throwaway diagnostic jobs, each with the 4-pack scan as a same-commit control. Those jobs carried
continue-on-error: true, declaredtimeout-minutes, and were deliberately kept out ofsemgrep_checks.needsso they could not gate the required check. All of them have been removed, so the net diff is just the path gate and the timeout.Automated checks I actually ran:
bin/hogli lint:workflows- 7/7 checks pass across 123 workflows, includingWF003-dorny-negation(which exists to catch exactly the negation footgun this filter relies on) andWF007-required-check-gates.hogli ci:preflight- 0 failures.Filterclass over 20 representative paths. Temporary test, not committed.Filter match cases (all pass)
Matches (job runs):
.github/workflows/ci-security.yaml,packages/quill/packages/charts/src/core/canvas-renderer.ts,tools/hogli-commands/hogli_commands/doctor.py,bin/sandbox,terraform/main.tf,uv.lock,.agents/skills/x/SKILL.md,playwright/foo.tsNo match (job skips):
posthog/api/team.py,products/foo/backend/api.py,frontend/src/App.tsx,services/mcp/src/index.ts,.semgrep/rules/security/a.yaml,rust/capture/src/main.rs,docs/internal/x.md,ee/api/x.py,common/x/y.py,nodejs/x.ts,livestream/main.go,cli/src/main.rsThe dotfile cases matter: the action sets
dot: true, so**does cover.github/**and.agents/**.The 92.5% figure comes from replaying
git log --first-parent -400 masterand checking each merge's changed files against the exclude list. This PR editsci-security.yaml, so the new filter matches andsemgrep-generalruns here. CI on this PR exercises the gate rather than skipping it.No new tests. The behavior here is a workflow path filter, and
bin/hogli lint:workflowsalready guards the conventions this change touches.Automatic notifications
Docs update
None needed.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
I pointed Claude (Claude Code, Opus 5) at a failing
semgrep-generaljob and asked how often it breaks and whether it could be fixed. The original investigation invoked no repo skills. Recreating it here invoked/writing-pr-descriptions.I (actually Claude) rebuilt this PR from one commit on a fresh branch off master. The net diff matches #77481 byte for byte, applied as a patch and verified against the old branch.
The first useful finding was that the job was not failing on a semgrep rule at all. GitHub surfaces a
timeout-minuteskill ascancelled, which is why this reads as noise rather than a bug. Confirming it meant checking that sibling jobs in the same run had completed successfully, ruling out a whole-run cancellation, then separating the ~20.2 min kills from genuine cancellations by duration across a week of runs.The first pass stopped there and shipped the gate plus the timeout, with a guess that the
p/trailofbitspack was simply expensive. That guess was wrong, and the 81s local vs 12 min CI gap was the thread worth pulling: a 14x difference is too large for hardware. Ruling things out one at a time (semgrep version on arm64, rule count, file set, engine errors, image architecture) left "something environmental changed at a point in time", which turned into plotting the job's duration back through the year and bisecting to a two minute window on Jun 24.Worth flagging for review: the code change here is unchanged from that first pass and is deliberately defensive rather than curative. It stops the bleeding without touching the security scanner pin. Whether to chase the version regression is a separate call, and reverting is specifically not safe given why #65921 happened.