From 8db9a1db797c51be7feb576513bc80c9df5540d1 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sun, 13 Sep 2026 13:04:42 +0900 Subject: [PATCH 1/2] fix(sast-semgrep): fold the changed-scope gate into its single consumer job One consumer, one runner: the standalone `changed-scope` job cost a second runner allocation per PR org-wide purely to compute two booleans for the `semgrep` job. The classifier now runs as a step inside `semgrep` (after harden-runner), the expensive steps gate on `steps.scope.outputs.code`, and the enforce step carries the same guard so a step-skipped scan's empty `rc` cannot fail a doc-only PR. The job keeps `if: github.event.action != 'closed'` with no needs-output term, so a doc-only run still concludes `success` (required-workflow-path-filter-boundary.md). strix.yml is left alone (hot-file collision zone). Measured in #1904; contract tests updated. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/sast-semgrep.yml | 50 ++++++++----------- docs/product-technical-gap-baseline.md | 21 +++++++- tests/test_docs_only_pr_runner_admission.py | 50 ++++++++++++++++--- ...required_security_runner_image_contract.py | 8 +-- 4 files changed, 89 insertions(+), 40 deletions(-) diff --git a/.github/workflows/sast-semgrep.yml b/.github/workflows/sast-semgrep.yml index 12b7013da3..283eac0097 100644 --- a/.github/workflows/sast-semgrep.yml +++ b/.github/workflows/sast-semgrep.yml @@ -38,8 +38,8 @@ permissions: contents: read jobs: - changed-scope: - name: Detect changed scope + semgrep: + name: Semgrep (multi-language SAST) # The org ruleset IGNORES every `on:` filter (paths, branches, types) when it # runs this workflow in another repository, and a trigger-level skip would # leave `.github`'s classic required contexts Pending forever. Both @@ -47,16 +47,26 @@ jobs: # here and consumed through `needs`. See # docs/doctoring/required-workflow-path-filter-boundary.md. # Fails OPEN: an unreadable, empty, or truncated file list scans everything. + # The gate lives inside this job as a step-level guard (one runner, not two). if: github.event.action != 'closed' runs-on: ubuntu-24.04 - timeout-minutes: 5 permissions: contents: read pull-requests: read - outputs: - code: ${{ steps.scope.outputs.code }} - deps: ${{ steps.scope.outputs.deps }} + security-events: write + actions: read + env: + # Deterministic, no telemetry: registry rules are fetched but no scan data + # is sent back. + SEMGREP_SEND_METRICS: "off" + # Semgrep OSS 1.169.0. Keep the immutable manifest reference in one + # place so hosted scans and local reproduction cannot drift. + SEMGREP_IMAGE: "semgrep/semgrep@sha256:2b33f46ba66cf8cc2ad59ccfa7d22951fd00c632c38f1339e84ec8e6e641a942" steps: + - name: Harden the runner (Audit all outbound calls) + uses: step-security/harden-runner@b09bb98e06d4d774595224525879c09bc6e98c40 # v2.20.1 + with: + egress-policy: audit - name: Classify changed paths id: scope env: @@ -99,35 +109,15 @@ jobs: echo "code=${code}" >> "$GITHUB_OUTPUT" echo "deps=${deps}" >> "$GITHUB_OUTPUT" echo "changed-scope code=${code} deps=${deps}" - - semgrep: - name: Semgrep (multi-language SAST) - needs: changed-scope - if: github.event.action != 'closed' && needs.changed-scope.outputs.code == 'true' - runs-on: ubuntu-24.04 - permissions: - contents: read - security-events: write - actions: read - env: - # Deterministic, no telemetry: registry rules are fetched but no scan data - # is sent back. - SEMGREP_SEND_METRICS: "off" - # Semgrep OSS 1.169.0. Keep the immutable manifest reference in one - # place so hosted scans and local reproduction cannot drift. - SEMGREP_IMAGE: "semgrep/semgrep@sha256:2b33f46ba66cf8cc2ad59ccfa7d22951fd00c632c38f1339e84ec8e6e641a942" - steps: - - name: Harden the runner (Audit all outbound calls) - uses: step-security/harden-runner@b09bb98e06d4d774595224525879c09bc6e98c40 # v2.20.1 - with: - egress-policy: audit - name: Checkout exact submitted revision + if: steps.scope.outputs.code == 'true' uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: repository: ${{ github.event.pull_request.head.repo.full_name || github.repository }} ref: ${{ github.event.pull_request.head.sha || github.sha }} persist-credentials: false - name: Verify exact submitted revision + if: steps.scope.outputs.code == 'true' env: EXPECTED_CHECKOUT_REPOSITORY: ${{ github.event.pull_request.head.repo.full_name || github.repository }} EXPECTED_CHECKOUT_SHA: ${{ github.event.pull_request.head.sha || github.sha }} @@ -140,6 +130,7 @@ jobs: fi echo "SAST_CHECKOUT scanner=semgrep repository=${EXPECTED_CHECKOUT_REPOSITORY} expected_sha=${EXPECTED_CHECKOUT_SHA} actual_sha=${actual_sha}" - name: Verify pinned Semgrep manifest + if: steps.scope.outputs.code == 'true' run: | set -euo pipefail if [[ "${SEMGREP_IMAGE}" =~ ^semgrep/semgrep@sha256:[0-9a-f]{64}$ ]]; then @@ -151,6 +142,7 @@ jobs: fi - name: Run Semgrep (SARIF) id: semgrep + if: steps.scope.outputs.code == 'true' run: | set +e echo "Using ${SEMGREP_IMAGE}" @@ -219,7 +211,7 @@ jobs: echo "SEMGREP_ENGINE_FAILURE rc=${SEMGREP_RC:-missing}: Semgrep failed without a WARNING/ERROR SARIF result; inspect the scan command output above." fi - name: Enforce Semgrep gate (fail on Medium+ findings) - if: always() && (steps.semgrep_sarif.outputs.finding_count != '0' || steps.semgrep.outputs.rc != '0') + if: always() && steps.scope.outputs.code == 'true' && (steps.semgrep_sarif.outputs.finding_count != '0' || steps.semgrep.outputs.rc != '0') env: SEMGREP_RC: ${{ steps.semgrep.outputs.rc }} SEMGREP_FINDING_COUNT: ${{ steps.semgrep_sarif.outputs.finding_count }} diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 1cc9e20313..966ac7ef25 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -3237,8 +3237,8 @@ intended contract before rewriting the assertion — left for a dedicated follow ## Items 15/16/17 measurement: `Detect changed scope` gate jobs — 2 of 3 are pure runner overhead — 2026-09-05 -**Status:** Measured, not yet fixed. Recorded so the fix is grounded in real numbers rather than the intuition -this measurement partly refuted. +**Status:** Measured 2026-09-05; `sast-semgrep.yml` fixed 2026-09-13 (below); `strix.yml` deferred. Recorded so +the fix is grounded in real numbers rather than the intuition this measurement partly refuted. **Why measured.** Items 15/16/17 ask to remove needlessly-triggered workflows, consolidate workflow files ("bootup에도 시간이 듦"), and cut redundant steps; the standing complaint is the org's 60-concurrent-job @@ -3353,3 +3353,20 @@ queries the check-runs API at its own time, order-independently. The implementin their change was safe because they had scoped it narrowly, not because they had checked for the name collision — which is the more useful lesson: **a job name is unique only within one workflow file, and the same name in another file can carry the opposite safety property.** + +**Fixed for `sast-semgrep.yml`, 2026-09-13.** The standalone `changed-scope` job is gone; its +"Classify changed paths" step now runs inside the single consumer `semgrep` (after `harden-runner`, +which must audit the classifier's own `gh api` egress) and the four expensive steps plus the final +"Enforce Semgrep gate" step carry `steps.scope.outputs.code == 'true'`. The job keeps +`if: github.event.action != 'closed'` with no `needs.` term, so a doc-only PR's run still executes one +job that concludes `success` -- the load-bearing property from +[`required-workflow-path-filter-boundary.md`](doctoring/required-workflow-path-filter-boundary.md) is +preserved, and neither `Detect changed scope` nor `Semgrep (multi-language SAST)` is among `.github`'s +classic required contexts, so nothing goes Pending there. One trap the first draft would have shipped: +the enforce step's `always() && (... || steps.semgrep.outputs.rc != '0')` evaluates `rc` as the empty +string when `Run Semgrep` is step-skipped, which is `!= '0'` and would have failed every doc-only PR; +the guard on that step is what makes the fold safe. Net: one runner allocation per PR for this +workflow instead of two, org-wide. `strix.yml` (the other single-consumer gate) is deliberately left +alone -- it is a documented multi-PR hot-file collision zone. Contract: +`tests/test_docs_only_pr_runner_admission.py::test_sast_semgrep_folds_the_gate_into_its_single_consumer_at_step_level`, +`tests/test_required_security_runner_image_contract.py`. diff --git a/tests/test_docs_only_pr_runner_admission.py b/tests/test_docs_only_pr_runner_admission.py index 49631d2a19..674b984b63 100644 --- a/tests/test_docs_only_pr_runner_admission.py +++ b/tests/test_docs_only_pr_runner_admission.py @@ -31,9 +31,11 @@ WORKFLOWS_DIR = REPO_ROOT / ".github/workflows" # The required workflows that keep the canonical `changed-scope` gate job. +# `sast-semgrep.yml` has only one consumer job, so it folds the classifier +# into that job as a step-level guard instead of a standalone job -- see +# GATED_JOBS below. GATE_WORKFLOWS = ( "security-scan.yml", - "sast-semgrep.yml", "strix.yml", ) @@ -52,7 +54,6 @@ # output, keyed by workflow filename. GATED_JOBS = { "security-scan.yml": ("osv-scan", "dependency-review", "trivy-fs", "scorecard"), - "sast-semgrep.yml": ("semgrep",), "strix.yml": ("strix",), } @@ -86,7 +87,7 @@ def _on_block(workflow: str) -> str: def test_gate_job_is_byte_identical_across_the_five_workflows_apart_from_if(): - """The `changed-scope` block must not drift between its five copies.""" + """The `changed-scope` block must not drift between every gate copy.""" normalized_blocks = set() for filename in GATE_WORKFLOWS: workflow = _read(filename) @@ -110,7 +111,7 @@ def test_gate_job_and_codeql_scope_step_share_one_doc_pattern_line(): `COPYING.txt`/`NOTICE`/`NOTICE.txt` names. """ doc_pattern_lines = set() - for filename in (*GATE_WORKFLOWS, "codeql-pr.yml"): + for filename in (*GATE_WORKFLOWS, "sast-semgrep.yml", "codeql-pr.yml"): workflow = _read(filename) matches = [ line for line in workflow.splitlines() if "*.md|*.markdown" in line @@ -208,8 +209,8 @@ def test_codeql_pr_gates_analyze_head_at_step_level_not_job_level(): def test_each_gate_workflow_keeps_an_always_admitted_job(): """A fully-skipped run must conclude `success`, never `skipped`. - Every one of the five workflows needs at least one job with no `needs:` - and no needs-output-dependent `if:` -- the `changed-scope` job itself + Every gate workflow needs at least one job with no `needs:` and no + needs-output-dependent `if:` -- the `changed-scope` job itself qualifies -- so a doc-only PR's run still has a job that runs and succeeds instead of every job skipping and the run itself reporting `skipped` (an undocumented conclusion for a required check). @@ -220,3 +221,40 @@ def test_each_gate_workflow_keeps_an_always_admitted_job(): job_if = re.search(r"(?m)^ if: (.*)$", block) assert job_if is not None, filename assert "needs." not in job_if.group(1), filename + + +def test_sast_semgrep_folds_the_gate_into_its_single_consumer_at_step_level(): + """`sast-semgrep.yml` has one consumer, so the gate is a step, not a job. + + A standalone `changed-scope` job cost a second runner allocation per PR + purely to compute two booleans for one downstream job (measured in + docs/product-technical-gap-baseline.md, "Items 15/16/17 measurement"). + Folding it into `semgrep` keeps the load-bearing property -- the job + still runs and concludes `success` on a doc-only PR -- while the + expensive steps gate on the classifier step's output. The final gate + step must also carry that guard: a step-skipped `Run Semgrep` leaves + `steps.semgrep.outputs.rc` empty, which is `!= '0'`. + """ + workflow = _read("sast-semgrep.yml") + # The classifier's own log lines keep saying "changed-scope" (byte-for-byte + # verbatim across every copy, see test_gate_job_and_codeql_scope_step_share_ + # one_doc_pattern_line); what must be gone is the standalone JOB. + assert "changed-scope:" not in workflow + assert "needs: changed-scope" not in workflow + assert "needs.changed-scope" not in workflow + assert workflow.count("runs-on: ubuntu-24.04") == 1 + + semgrep = _top_level_job_block(workflow, "semgrep") + assert not re.search(r"(?m)^ needs:", semgrep) + job_if = re.search(r"(?m)^ if: (.*)$", semgrep) + assert job_if is not None + assert job_if.group(1) == "github.event.action != 'closed'" + assert "pull-requests: read" in semgrep + assert "id: scope" in semgrep + assert semgrep.count("steps.scope.outputs.code == 'true'") == 5 + assert ( + "if: always() && steps.scope.outputs.code == 'true' && " + "(steps.semgrep_sarif.outputs.finding_count != '0' || steps.semgrep.outputs.rc != '0')" + ) in semgrep + # Harden-runner audits egress and must precede the classifier's gh api call. + assert semgrep.index("Harden the runner") < semgrep.index("Classify changed paths") diff --git a/tests/test_required_security_runner_image_contract.py b/tests/test_required_security_runner_image_contract.py index 2b48f66251..d20c0c3a98 100644 --- a/tests/test_required_security_runner_image_contract.py +++ b/tests/test_required_security_runner_image_contract.py @@ -28,13 +28,15 @@ def test_sast_semgrep_uses_explicit_supported_image(self) -> None: `#1656` removed the sibling `cancel-closed-pr-runs` no-op job (it only duplicated PR-stable workflow concurrency), leaving one runner - job in this workflow instead of two. It is 2, not 1, again after the + job in this workflow instead of two. It was 2, not 1, again after the `changed-scope` gate job was added to skip doc-only PR scope (org - ruleset 18156473 ignores trigger-level path filters). + ruleset 18156473 ignores trigger-level path filters). The count + returned to 1 when that `changed-scope` job was folded into the + `semgrep` job as a step-level guard (one consumer, one runner). """ workflow = SAST_SEMGREP.read_text(encoding="utf-8") self.assertNotIn("runs-on: ubuntu-latest", workflow) - self.assertEqual(workflow.count("runs-on: ubuntu-24.04"), 2) + self.assertEqual(workflow.count("runs-on: ubuntu-24.04"), 1) if __name__ == "__main__": From fb8138e6189e8a931fa937675f2c990421ba1dac Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sun, 13 Sep 2026 13:29:44 +0900 Subject: [PATCH 2/2] docs(sast-semgrep): describe the step-level guard in the job comment The comment still said the classifier verdict was consumed through `needs`; nothing consumes it that way any more. Co-Authored-By: Claude Fable 5.1 --- .github/workflows/sast-semgrep.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/sast-semgrep.yml b/.github/workflows/sast-semgrep.yml index 283eac0097..f8ab04b865 100644 --- a/.github/workflows/sast-semgrep.yml +++ b/.github/workflows/sast-semgrep.yml @@ -43,8 +43,9 @@ jobs: # The org ruleset IGNORES every `on:` filter (paths, branches, types) when it # runs this workflow in another repository, and a trigger-level skip would # leave `.github`'s classic required contexts Pending forever. Both - # mechanisms honour a JOB-level skip, so the doc/image-only decision is made - # here and consumed through `needs`. See + # mechanisms honour a job that runs and concludes on its own, so the + # doc/image-only decision is made by the classifier step below and consumed + # by the expensive steps' `if:` guards. See # docs/doctoring/required-workflow-path-filter-boundary.md. # Fails OPEN: an unreadable, empty, or truncated file list scans everything. # The gate lives inside this job as a step-level guard (one runner, not two).