From c20888dfbf80cf0db96cf85ead70f1d09fb70117 Mon Sep 17 00:00:00 2001 From: David Yaffe Date: Mon, 17 Aug 2026 13:46:47 -0400 Subject: [PATCH 01/14] Gate PRs on the full test suite, not just after merge coverage.yml runs make test-all (all-features + failpoints + every broker backend) only on push to main, so broken optional features or broker integrations land on main before anyone notices. Add a pull_request-triggered workflow that runs the same suite before merge. --- .github/workflows/full-tests.yml | 143 +++++++++++++++++++++++++++++++ 1 file changed, 143 insertions(+) create mode 100644 .github/workflows/full-tests.yml diff --git a/.github/workflows/full-tests.yml b/.github/workflows/full-tests.yml new file mode 100644 index 00000000000..b11443f581d --- /dev/null +++ b/.github/workflows/full-tests.yml @@ -0,0 +1,143 @@ +name: Full test suite + +# Runs the full test suite (`make test-all`: --all-features + failpoints) +# against every broker/backend service (Kafka, Pulsar, Azurite, fake GCS, +# Pub/Sub emulator, LocalStack, Postgres) *before* merge. +# +# Today only `ci.yml` gates PRs, and it runs `cargo nextest --features=postgres,metrics` +# on `ubuntu-latest` with a single Postgres service container. None of the +# other optional features (kafka, pulsar, sqs, gcp-pubsub, azure, gcs, +# datafusion, failpoints, ...) or broker-backed integration tests are +# compiled or exercised pre-merge. The only workflow that does run them +# (`coverage.yml`) triggers on `push` to `main`, i.e. *after* the PR already +# merged. This workflow closes that gap by running the same `make test-all` +# target documented in CLAUDE.md directly on pull requests, so a broken +# feature or broker integration blocks the merge instead of breaking main +# after the fact. +on: + workflow_dispatch: + pull_request: + push: + branches: + - main + - trigger-ci-workflow + paths: + - "quickwit/**" + - "!quickwit/quickwit-ui/**" + +permissions: + contents: read + +env: + AWS_REGION: us-east-1 + AWS_ACCESS_KEY_ID: "placeholder" + AWS_SECRET_ACCESS_KEY: "placeholder" + CARGO_INCREMENTAL: 0 + RUST_BACKTRACE: 1 + RUSTFLAGS: -Dwarnings --cfg tokio_unstable + # Services started by `make docker-compose-up` expose themselves on + # localhost since the job (not the services) is running on the runner host. + QW_S3_ENDPOINT: "http://localhost:4566" + QW_S3_FORCE_PATH_STYLE_ACCESS: 1 + PUBSUB_EMULATOR_HOST: "localhost:8681" + # Only the backends exercised by the test suite — skips the + # observability stack (jaeger/grafana/otel-collector/prometheus). + DOCKER_SERVICES: "localstack,postgres,kafka-broker,pulsar-broker,azurite,fake-gcs-server,gcp-pubsub-emulator" + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +jobs: + full-tests: + name: All-features tests + failpoints (make test-all) + runs-on: gh-ubuntu-arm64 + timeout-minutes: 60 + permissions: + contents: read + actions: write + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + + - uses: dorny/paths-filter@fbd0ab8f3e69293af611ebaee6363fc25e6d187d # v4.0.1 + id: modified + with: + filters: | + rust_src: + - quickwit/** + - Makefile + - docker-compose.yml + - .github/workflows/full-tests.yml + - "!quickwit/quickwit-ui/**" + + - name: Install Ubuntu packages + if: steps.modified.outputs.rust_src == 'true' + run: | + sudo apt-get update + sudo apt-get -y install protobuf-compiler libsasl2-dev libcurl4-openssl-dev + + - uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v.6.2.0 + if: steps.modified.outputs.rust_src == 'true' + with: + python-version: '3.11' + + - name: Setup stable Rust Toolchain + if: steps.modified.outputs.rust_src == 'true' + uses: dtolnay/rust-toolchain@3c5f7ea28cd621ae0bf5283f0e981fb97b8a7af9 # master + with: + toolchain: stable + + - name: Setup cache + if: steps.modified.outputs.rust_src == 'true' + uses: Swatinem/rust-cache@c19371144df3bb44fab255c43d04cbc2ab54d1c4 # v2.9.1 + with: + workspaces: "./quickwit -> target" + shared-key: "quickwit-cargo-full" + + - name: Install cargo-nextest + if: steps.modified.outputs.rust_src == 'true' + uses: taiki-e/install-action@7769b73c2ec98c38dfcf2e18c83cfd4880c038c1 + with: + tool: cargo-nextest + + - name: Start Docker services + if: steps.modified.outputs.rust_src == 'true' + run: make docker-compose-up + + - name: Install python packages + if: steps.modified.outputs.rust_src == 'true' + run: | + pip install --user --require-hashes -r ${{ github.workspace }}/.github/workflows/requirements.txt + pipenv install --deploy --ignore-pipfile + working-directory: ./quickwit/quickwit-cli/tests + + - name: Prepare LocalStack S3 + if: steps.modified.outputs.rust_src == 'true' + run: pipenv run ./prepare_tests.sh + working-directory: ./quickwit/quickwit-cli/tests + + - name: make test-all + if: always() && steps.modified.outputs.rust_src == 'true' + run: make -C quickwit test-all + env: + QW_TEST_DATABASE_URL: postgres://quickwit-dev:quickwit-dev@localhost:5432/quickwit-metastore-dev + + on-failure: + if: ${{ github.repository_owner == 'quickwit-oss' && failure() }} + name: On Failure + needs: [full-tests] + runs-on: ubuntu-latest + steps: + - name: Send Message + uses: sarisia/actions-status-discord@eb045afee445dc055c18d3d90bd0f244fd062708 # v1.16.0 + with: + webhook: ${{ secrets.DISCORD_WEBHOOK }} + nodetail: true + color: "#FF0000" + title: "" + description: | + ### ❌ [${{ github.event.pull_request.title }}](${{ github.event.pull_request.html_url }}) + + @${{ github.actor }} the full test suite (`make test-all`) failed on your PR. + + **[View logs](https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }})** From 26b87b80213f1633f305043f961f5b6f15e23cff Mon Sep 17 00:00:00 2001 From: David Yaffe Date: Mon, 17 Aug 2026 14:32:08 -0400 Subject: [PATCH 02/14] Install protoc via taiki-e/install-action instead of apt apt's protobuf-compiler doesn't support proto3 optional fields by default, which the substrait crate (pulled in by --all-features via the datafusion feature) requires. coverage.yml already works around this the same way; this job needs it too since it now builds with --all-features. --- .github/workflows/full-tests.yml | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/.github/workflows/full-tests.yml b/.github/workflows/full-tests.yml index b11443f581d..7feee967e3f 100644 --- a/.github/workflows/full-tests.yml +++ b/.github/workflows/full-tests.yml @@ -74,7 +74,17 @@ jobs: if: steps.modified.outputs.rust_src == 'true' run: | sudo apt-get update - sudo apt-get -y install protobuf-compiler libsasl2-dev libcurl4-openssl-dev + sudo apt-get -y install libsasl2-dev libcurl4-openssl-dev + + # apt's protobuf-compiler is too old to support proto3 optional fields + # by default, which the `substrait` crate (pulled in by --all-features + # via the `datafusion` feature) requires. coverage.yml hits the same + # constraint and installs protoc this way for the same reason. + - name: Install protoc + if: steps.modified.outputs.rust_src == 'true' + uses: taiki-e/install-action@7769b73c2ec98c38dfcf2e18c83cfd4880c038c1 + with: + tool: protoc - uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v.6.2.0 if: steps.modified.outputs.rust_src == 'true' From f711954e3f2e2dcab5223420280f910f9d8d7cc3 Mon Sep 17 00:00:00 2001 From: David Yaffe Date: Mon, 17 Aug 2026 16:11:08 -0400 Subject: [PATCH 03/14] Gate full-tests.yml behind a maintainer PR-review command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Running the full broker/all-features suite unconditionally on every PR push doesn't match how the team already runs CI on the sibling vector repo: there, the equivalent full suite is merge-queue- or maintainer-comment-gated, never automatic-for-everyone. full-tests.yml now only runs via workflow_dispatch. The new full-tests-trigger.yml fires on a submitted PR review whose body starts with /ci-run-full-tests, checks the reviewer's actual repos.getCollaboratorPermissionLevel (maintain/admin — not author_association, which can't distinguish write-only collaborators from maintainers), sets a pending commit status, and dispatches the run against the reviewed commit. It's still a required status check: since only maintain/admin reviewers can trigger it and that's also who can merge, a PR can't merge without that same person having run and passed the full suite themselves. --- .github/workflows/full-tests-trigger.yml | 87 ++++++++++++++++++ .github/workflows/full-tests.yml | 107 ++++++++++++++--------- 2 files changed, 152 insertions(+), 42 deletions(-) create mode 100644 .github/workflows/full-tests-trigger.yml diff --git a/.github/workflows/full-tests-trigger.yml b/.github/workflows/full-tests-trigger.yml new file mode 100644 index 00000000000..a1b0a7faa5b --- /dev/null +++ b/.github/workflows/full-tests-trigger.yml @@ -0,0 +1,87 @@ +name: Full test suite trigger + +# `full-tests.yml` ("full-tests / make test-all") is a required status check +# but never runs on its own — see the comment there for why. This workflow +# is the trigger: a reviewer submits a PR review whose body starts with +# `/ci-run-full-tests`, and if that reviewer actually holds `maintain` or +# `admin` permission on this repo, this sets a pending status on the +# reviewed commit and dispatches full-tests.yml against it. +# +# Deliberately checks the real collaborator permission level via the API +# rather than `github.event.review.author_association` — that field can +# only tell you OWNER/MEMBER/COLLABORATOR/etc, none of which distinguish +# "has write access" from "has maintain/admin access". As of 2026-08-17, +# quickwit-oss/quickwit has 57 collaborators with at least write access but +# only 24 with maintain/admin — author_association would have let all 57 +# trigger this, not just the intended 24. +on: + pull_request_review: + types: [submitted] + +permissions: + contents: read + statuses: write + actions: write + +concurrency: + group: full-tests-trigger-${{ github.event.review.commit_id }} + cancel-in-progress: false + +jobs: + trigger: + name: Trigger full test suite + runs-on: ubuntu-latest + if: ${{ startsWith(github.event.review.body, '/ci-run-full-tests') }} + timeout-minutes: 5 + steps: + - name: Check reviewer has maintain/admin permission + uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + with: + script: | + const { data } = await github.rest.repos.getCollaboratorPermissionLevel({ + owner: context.repo.owner, + repo: context.repo.repo, + username: context.actor, + }); + core.info(`${context.actor} has permission: ${data.permission}`); + if (!['maintain', 'admin'].includes(data.permission)) { + core.setFailed( + `@${context.actor} has '${data.permission}' permission on this repo, ` + + `but triggering the full test suite requires 'maintain' or 'admin'.` + ); + } + + - name: Set commit status to pending + uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + with: + script: | + await github.rest.repos.createCommitStatus({ + owner: context.repo.owner, + repo: context.repo.repo, + sha: '${{ github.event.review.commit_id }}', + state: 'pending', + context: 'full-tests / make test-all', + description: `Triggered by @${context.actor}`, + target_url: `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/pull/${{ github.event.pull_request.number }}`, + }) + + # Dispatch against `main` (a ref guaranteed to exist in this repo), + # not the PR's own branch: for fork PRs, the head branch name only + # exists in the fork, not here, and workflow_dispatch's `ref` must + # name a branch/tag in *this* repo. full-tests.yml's own checkout + # step is what actually fetches `inputs.sha` — that's the exact + # commit under review, regardless of which ref ran the dispatch. + - name: Dispatch full-tests.yml + uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + with: + script: | + await github.rest.actions.createWorkflowDispatch({ + owner: context.repo.owner, + repo: context.repo.repo, + workflow_id: 'full-tests.yml', + ref: 'main', + inputs: { + sha: '${{ github.event.review.commit_id }}', + pr_number: '${{ github.event.pull_request.number }}', + }, + }) diff --git a/.github/workflows/full-tests.yml b/.github/workflows/full-tests.yml index 7feee967e3f..481f5f0ea0e 100644 --- a/.github/workflows/full-tests.yml +++ b/.github/workflows/full-tests.yml @@ -2,31 +2,42 @@ name: Full test suite # Runs the full test suite (`make test-all`: --all-features + failpoints) # against every broker/backend service (Kafka, Pulsar, Azurite, fake GCS, -# Pub/Sub emulator, LocalStack, Postgres) *before* merge. +# Pub/Sub emulator, LocalStack, Postgres). # -# Today only `ci.yml` gates PRs, and it runs `cargo nextest --features=postgres,metrics` -# on `ubuntu-latest` with a single Postgres service container. None of the -# other optional features (kafka, pulsar, sqs, gcp-pubsub, azure, gcs, -# datafusion, failpoints, ...) or broker-backed integration tests are -# compiled or exercised pre-merge. The only workflow that does run them -# (`coverage.yml`) triggers on `push` to `main`, i.e. *after* the PR already -# merged. This workflow closes that gap by running the same `make test-all` -# target documented in CLAUDE.md directly on pull requests, so a broken -# feature or broker integration blocks the merge instead of breaking main -# after the fact. +# This is a REQUIRED status check for merging, but it never runs on its +# own — it only runs via workflow_dispatch, triggered by +# full-tests-trigger.yml when a reviewer with `maintain`/`admin` permission +# on the repo submits a PR review containing `/ci-run-full-tests`. That +# reviewer is also the one who can merge, so the required check can't be +# bypassed: they can't hit merge until they've explicitly run (and passed) +# the full suite themselves. +# +# Why gate it at all instead of running automatically on every PR: only +# `ci.yml` (`cargo nextest --features=postgres,metrics`) and +# `datafusion-ci.yml` run automatically. None of the other optional +# features (kafka, pulsar, sqs, gcp-pubsub, azure, gcs, failpoints, ...) or +# broker-backed integration tests get compiled/exercised otherwise pre-merge +# — the only workflow that already runs them (`coverage.yml`) triggers on +# `push` to `main`, i.e. after merge. Running the full suite unconditionally +# on every PR push was the first design here, but it doesn't match how +# quickwit's team already runs the sibling `vector` repo's CI (broker/full +# suites there are merge-queue- or maintainer-comment-gated, never +# automatic-for-everyone) — this mirrors that instead. on: workflow_dispatch: - pull_request: - push: - branches: - - main - - trigger-ci-workflow - paths: - - "quickwit/**" - - "!quickwit/quickwit-ui/**" + inputs: + sha: + description: "Commit SHA to check out and report the status against" + required: true + type: string + pr_number: + description: "PR number (used only for the failure notification link)" + required: true + type: string permissions: contents: read + statuses: write env: AWS_REGION: us-east-1 @@ -43,9 +54,10 @@ env: # Only the backends exercised by the test suite — skips the # observability stack (jaeger/grafana/otel-collector/prometheus). DOCKER_SERVICES: "localstack,postgres,kafka-broker,pulsar-broker,azurite,fake-gcs-server,gcp-pubsub-emulator" + STATUS_CONTEXT: "full-tests / make test-all" concurrency: - group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + group: ${{ github.workflow }}-${{ inputs.sha }} cancel-in-progress: true jobs: @@ -55,23 +67,27 @@ jobs: timeout-minutes: 60 permissions: contents: read - actions: write + statuses: write steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + ref: ${{ inputs.sha }} - - uses: dorny/paths-filter@fbd0ab8f3e69293af611ebaee6363fc25e6d187d # v4.0.1 - id: modified + - name: Set commit status to pending + uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 with: - filters: | - rust_src: - - quickwit/** - - Makefile - - docker-compose.yml - - .github/workflows/full-tests.yml - - "!quickwit/quickwit-ui/**" + script: | + await github.rest.repos.createCommitStatus({ + owner: context.repo.owner, + repo: context.repo.repo, + sha: '${{ inputs.sha }}', + state: 'pending', + context: '${{ env.STATUS_CONTEXT }}', + description: 'Running full test suite...', + target_url: `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`, + }) - name: Install Ubuntu packages - if: steps.modified.outputs.rust_src == 'true' run: | sudo apt-get update sudo apt-get -y install libsasl2-dev libcurl4-openssl-dev @@ -81,57 +97,64 @@ jobs: # via the `datafusion` feature) requires. coverage.yml hits the same # constraint and installs protoc this way for the same reason. - name: Install protoc - if: steps.modified.outputs.rust_src == 'true' uses: taiki-e/install-action@7769b73c2ec98c38dfcf2e18c83cfd4880c038c1 with: tool: protoc - uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v.6.2.0 - if: steps.modified.outputs.rust_src == 'true' with: python-version: '3.11' - name: Setup stable Rust Toolchain - if: steps.modified.outputs.rust_src == 'true' uses: dtolnay/rust-toolchain@3c5f7ea28cd621ae0bf5283f0e981fb97b8a7af9 # master with: toolchain: stable - name: Setup cache - if: steps.modified.outputs.rust_src == 'true' uses: Swatinem/rust-cache@c19371144df3bb44fab255c43d04cbc2ab54d1c4 # v2.9.1 with: workspaces: "./quickwit -> target" shared-key: "quickwit-cargo-full" - name: Install cargo-nextest - if: steps.modified.outputs.rust_src == 'true' uses: taiki-e/install-action@7769b73c2ec98c38dfcf2e18c83cfd4880c038c1 with: tool: cargo-nextest - name: Start Docker services - if: steps.modified.outputs.rust_src == 'true' run: make docker-compose-up - name: Install python packages - if: steps.modified.outputs.rust_src == 'true' run: | pip install --user --require-hashes -r ${{ github.workspace }}/.github/workflows/requirements.txt pipenv install --deploy --ignore-pipfile working-directory: ./quickwit/quickwit-cli/tests - name: Prepare LocalStack S3 - if: steps.modified.outputs.rust_src == 'true' run: pipenv run ./prepare_tests.sh working-directory: ./quickwit/quickwit-cli/tests - name: make test-all - if: always() && steps.modified.outputs.rust_src == 'true' run: make -C quickwit test-all env: QW_TEST_DATABASE_URL: postgres://quickwit-dev:quickwit-dev@localhost:5432/quickwit-metastore-dev + - name: Report final status + if: always() + uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + with: + script: | + const state = '${{ job.status }}' === 'success' ? 'success' : 'failure'; + await github.rest.repos.createCommitStatus({ + owner: context.repo.owner, + repo: context.repo.repo, + sha: '${{ inputs.sha }}', + state, + context: '${{ env.STATUS_CONTEXT }}', + description: state === 'success' ? 'Full test suite passed' : 'Full test suite failed', + target_url: `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`, + }) + on-failure: if: ${{ github.repository_owner == 'quickwit-oss' && failure() }} name: On Failure @@ -146,8 +169,8 @@ jobs: color: "#FF0000" title: "" description: | - ### ❌ [${{ github.event.pull_request.title }}](${{ github.event.pull_request.html_url }}) + ### ❌ [PR #${{ inputs.pr_number }}](https://github.com/${{ github.repository }}/pull/${{ inputs.pr_number }}) - @${{ github.actor }} the full test suite (`make test-all`) failed on your PR. + The full test suite (`make test-all`) failed. **[View logs](https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }})** From b94b216260feefa8f2761af7efe39d030e49990e Mon Sep 17 00:00:00 2001 From: David Yaffe Date: Mon, 17 Aug 2026 16:18:52 -0400 Subject: [PATCH 04/14] Fix injection/auth-bypass findings from automated security review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit full-tests.yml: - Split status-posting into its own jobs (set-pending, report-status) so the job that checks out and runs untrusted PR content (full-tests) never holds statuses:write — previously that job could have forged a passing status onto any commit via the token persisted by actions/checkout during `make test-all`. - persist-credentials: false on that checkout, since it doesn't need git credentials at all. - Validate inputs.sha/inputs.pr_number to a strict shape (validate-inputs) before any other job trusts them, and pass them to github-script via env:/process.env rather than templating into script source — workflow_dispatch inputs are free-text fields anyone with repo write access can set to anything via the Actions UI/API, so a crafted value could otherwise break out of a string literal in a step holding statuses:write. full-tests-trigger.yml: - Bind the permission check to github.event.review.user.login (who actually submitted the review) instead of context.actor (whoever triggered this workflow run) — those differ on a manual re-run, where actor becomes the re-runner while the review payload stays frozen from the original event. --- .github/workflows/full-tests-trigger.yml | 29 ++++-- .github/workflows/full-tests.yml | 108 +++++++++++++++-------- 2 files changed, 93 insertions(+), 44 deletions(-) diff --git a/.github/workflows/full-tests-trigger.yml b/.github/workflows/full-tests-trigger.yml index a1b0a7faa5b..3a5ee74d62b 100644 --- a/.github/workflows/full-tests-trigger.yml +++ b/.github/workflows/full-tests-trigger.yml @@ -34,34 +34,46 @@ jobs: if: ${{ startsWith(github.event.review.body, '/ci-run-full-tests') }} timeout-minutes: 5 steps: + # Bound to github.event.review.user.login (who actually submitted the + # review), not context.actor (whoever triggered this workflow *run*). + # Those differ if someone later hits "Re-run all jobs": actor becomes + # the re-runner while review.body/commit_id/user stay frozen from the + # original event, which would otherwise let the re-runner's + # permission stand in for the original reviewer's. - name: Check reviewer has maintain/admin permission uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + env: + REVIEWER: ${{ github.event.review.user.login }} with: script: | + const reviewer = process.env.REVIEWER; const { data } = await github.rest.repos.getCollaboratorPermissionLevel({ owner: context.repo.owner, repo: context.repo.repo, - username: context.actor, + username: reviewer, }); - core.info(`${context.actor} has permission: ${data.permission}`); + core.info(`${reviewer} has permission: ${data.permission}`); if (!['maintain', 'admin'].includes(data.permission)) { core.setFailed( - `@${context.actor} has '${data.permission}' permission on this repo, ` + + `@${reviewer} has '${data.permission}' permission on this repo, ` + `but triggering the full test suite requires 'maintain' or 'admin'.` ); } - name: Set commit status to pending uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + env: + REVIEWER: ${{ github.event.review.user.login }} + SHA: ${{ github.event.review.commit_id }} with: script: | await github.rest.repos.createCommitStatus({ owner: context.repo.owner, repo: context.repo.repo, - sha: '${{ github.event.review.commit_id }}', + sha: process.env.SHA, state: 'pending', context: 'full-tests / make test-all', - description: `Triggered by @${context.actor}`, + description: `Triggered by @${process.env.REVIEWER}`, target_url: `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/pull/${{ github.event.pull_request.number }}`, }) @@ -73,6 +85,9 @@ jobs: # commit under review, regardless of which ref ran the dispatch. - name: Dispatch full-tests.yml uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + env: + SHA: ${{ github.event.review.commit_id }} + PR_NUMBER: ${{ github.event.pull_request.number }} with: script: | await github.rest.actions.createWorkflowDispatch({ @@ -81,7 +96,7 @@ jobs: workflow_id: 'full-tests.yml', ref: 'main', inputs: { - sha: '${{ github.event.review.commit_id }}', - pr_number: '${{ github.event.pull_request.number }}', + sha: process.env.SHA, + pr_number: process.env.PR_NUMBER, }, }) diff --git a/.github/workflows/full-tests.yml b/.github/workflows/full-tests.yml index 481f5f0ea0e..89ab32131eb 100644 --- a/.github/workflows/full-tests.yml +++ b/.github/workflows/full-tests.yml @@ -7,10 +7,7 @@ name: Full test suite # This is a REQUIRED status check for merging, but it never runs on its # own — it only runs via workflow_dispatch, triggered by # full-tests-trigger.yml when a reviewer with `maintain`/`admin` permission -# on the repo submits a PR review containing `/ci-run-full-tests`. That -# reviewer is also the one who can merge, so the required check can't be -# bypassed: they can't hit merge until they've explicitly run (and passed) -# the full suite themselves. +# on the repo submits a PR review containing `/ci-run-full-tests`. # # Why gate it at all instead of running automatically on every PR: only # `ci.yml` (`cargo nextest --features=postgres,metrics`) and @@ -23,6 +20,16 @@ name: Full test suite # quickwit's team already runs the sibling `vector` repo's CI (broker/full # suites there are merge-queue- or maintainer-comment-gated, never # automatic-for-everyone) — this mirrors that instead. +# +# Job layout is deliberately split so that the only job which checks out +# and executes untrusted PR content (`full-tests`) never holds +# `statuses: write`. `inputs.sha`/`inputs.pr_number` are workflow_dispatch +# inputs — free-text fields anyone with plain repo write access can set to +# anything via the Actions UI/API, bypassing full-tests-trigger.yml's +# permission check entirely — so they're validated to a strict shape +# (`validate-inputs`) before any other job trusts them, and passed to +# github-script via `env:`/`process.env` rather than templated into script +# source, so a crafted input can't break out of a string literal. on: workflow_dispatch: inputs: @@ -37,23 +44,8 @@ on: permissions: contents: read - statuses: write env: - AWS_REGION: us-east-1 - AWS_ACCESS_KEY_ID: "placeholder" - AWS_SECRET_ACCESS_KEY: "placeholder" - CARGO_INCREMENTAL: 0 - RUST_BACKTRACE: 1 - RUSTFLAGS: -Dwarnings --cfg tokio_unstable - # Services started by `make docker-compose-up` expose themselves on - # localhost since the job (not the services) is running on the runner host. - QW_S3_ENDPOINT: "http://localhost:4566" - QW_S3_FORCE_PATH_STYLE_ACCESS: 1 - PUBSUB_EMULATOR_HOST: "localhost:8681" - # Only the backends exercised by the test suite — skips the - # observability stack (jaeger/grafana/otel-collector/prometheus). - DOCKER_SERVICES: "localstack,postgres,kafka-broker,pulsar-broker,azurite,fake-gcs-server,gcp-pubsub-emulator" STATUS_CONTEXT: "full-tests / make test-all" concurrency: @@ -61,32 +53,62 @@ concurrency: cancel-in-progress: true jobs: - full-tests: - name: All-features tests + failpoints (make test-all) - runs-on: gh-ubuntu-arm64 - timeout-minutes: 60 + validate-inputs: + name: Validate inputs + runs-on: ubuntu-latest + timeout-minutes: 2 + permissions: {} + steps: + - name: Validate sha and pr_number are well-formed + env: + SHA: ${{ inputs.sha }} + PR_NUMBER: ${{ inputs.pr_number }} + run: | + if ! [[ "$SHA" =~ ^[0-9a-fA-F]{40}$ ]]; then + echo "::error::sha must be a 40-character hex commit SHA, got: $SHA" + exit 1 + fi + if ! [[ "$PR_NUMBER" =~ ^[0-9]+$ ]]; then + echo "::error::pr_number must be numeric, got: $PR_NUMBER" + exit 1 + fi + + set-pending: + name: Set pending status + needs: [validate-inputs] + runs-on: ubuntu-latest + timeout-minutes: 2 permissions: - contents: read statuses: write steps: - - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - with: - ref: ${{ inputs.sha }} - - - name: Set commit status to pending - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + env: + SHA: ${{ inputs.sha }} with: script: | await github.rest.repos.createCommitStatus({ owner: context.repo.owner, repo: context.repo.repo, - sha: '${{ inputs.sha }}', + sha: process.env.SHA, state: 'pending', context: '${{ env.STATUS_CONTEXT }}', description: 'Running full test suite...', target_url: `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`, }) + full-tests: + name: All-features tests + failpoints (make test-all) + needs: [set-pending] + runs-on: gh-ubuntu-arm64 + timeout-minutes: 60 + permissions: + contents: read + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + ref: ${{ inputs.sha }} + persist-credentials: false + - name: Install Ubuntu packages run: | sudo apt-get update @@ -139,16 +161,26 @@ jobs: env: QW_TEST_DATABASE_URL: postgres://quickwit-dev:quickwit-dev@localhost:5432/quickwit-metastore-dev - - name: Report final status - if: always() - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + report-status: + name: Report final status + needs: [validate-inputs, full-tests] + if: ${{ always() && needs.validate-inputs.result == 'success' }} + runs-on: ubuntu-latest + timeout-minutes: 2 + permissions: + statuses: write + steps: + - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + env: + SHA: ${{ inputs.sha }} + RESULT: ${{ needs.full-tests.result }} with: script: | - const state = '${{ job.status }}' === 'success' ? 'success' : 'failure'; + const state = process.env.RESULT === 'success' ? 'success' : 'failure'; await github.rest.repos.createCommitStatus({ owner: context.repo.owner, repo: context.repo.repo, - sha: '${{ inputs.sha }}', + sha: process.env.SHA, state, context: '${{ env.STATUS_CONTEXT }}', description: state === 'success' ? 'Full test suite passed' : 'Full test suite failed', @@ -156,10 +188,12 @@ jobs: }) on-failure: - if: ${{ github.repository_owner == 'quickwit-oss' && failure() }} + if: ${{ github.repository_owner == 'quickwit-oss' && needs.full-tests.result == 'failure' }} name: On Failure needs: [full-tests] runs-on: ubuntu-latest + timeout-minutes: 2 + permissions: {} steps: - name: Send Message uses: sarisia/actions-status-discord@eb045afee445dc055c18d3d90bd0f244fd062708 # v1.16.0 From 6e9bc5099b52c8af17d2d9d1b37e02ac9aa2a321 Mon Sep 17 00:00:00 2001 From: David Yaffe Date: Mon, 17 Aug 2026 16:26:20 -0400 Subject: [PATCH 05/14] Switch trigger to a plain PR comment instead of a submitted review pull_request_review requires using Files changed -> Review changes -> Submit review with the command as the review body; a normal comment on the Conversation tab (issue_comment) doesn't fire it, which is exactly how the first real attempt to use this failed. issue_comment has no commit sha the way a submitted review does, so this resolves the PR's current head sha itself via pulls.get before posting status or dispatching. --- .github/workflows/full-tests-trigger.yml | 80 +++++++++++++++--------- 1 file changed, 51 insertions(+), 29 deletions(-) diff --git a/.github/workflows/full-tests-trigger.yml b/.github/workflows/full-tests-trigger.yml index 3a5ee74d62b..ae50b7345f1 100644 --- a/.github/workflows/full-tests-trigger.yml +++ b/.github/workflows/full-tests-trigger.yml @@ -2,21 +2,30 @@ name: Full test suite trigger # `full-tests.yml` ("full-tests / make test-all") is a required status check # but never runs on its own — see the comment there for why. This workflow -# is the trigger: a reviewer submits a PR review whose body starts with -# `/ci-run-full-tests`, and if that reviewer actually holds `maintain` or -# `admin` permission on this repo, this sets a pending status on the -# reviewed commit and dispatches full-tests.yml against it. +# is the trigger: a PR comment whose body starts with `/ci-run-full-tests`, +# and if that commenter actually holds `maintain` or `admin` permission on +# this repo, this sets a pending status on the PR's current head commit and +# dispatches full-tests.yml against it. +# +# Uses issue_comment (a plain PR comment on the Conversation tab), not +# pull_request_review — a submitted review pins naturally to a specific +# commit (github.event.review.commit_id); a plain comment doesn't carry a +# commit sha at all, so this fetches the PR's current head sha itself. That +# means there's a small window between commenting and this job running +# where a new push could move the head sha out from under the comment — +# accepted as a UX tradeoff for "just comment normally" over requiring the +# Review-changes flow. # # Deliberately checks the real collaborator permission level via the API -# rather than `github.event.review.author_association` — that field can +# rather than `github.event.comment.author_association` — that field can # only tell you OWNER/MEMBER/COLLABORATOR/etc, none of which distinguish # "has write access" from "has maintain/admin access". As of 2026-08-17, # quickwit-oss/quickwit has 57 collaborators with at least write access but # only 24 with maintain/admin — author_association would have let all 57 # trigger this, not just the intended 24. on: - pull_request_review: - types: [submitted] + issue_comment: + types: [created] permissions: contents: read @@ -24,38 +33,51 @@ permissions: actions: write concurrency: - group: full-tests-trigger-${{ github.event.review.commit_id }} + group: full-tests-trigger-${{ github.event.comment.id }} cancel-in-progress: false jobs: trigger: name: Trigger full test suite runs-on: ubuntu-latest - if: ${{ startsWith(github.event.review.body, '/ci-run-full-tests') }} + # github.event.issue.pull_request only exists when the comment is on a + # PR (issue_comment fires for both issues and PRs). + if: ${{ github.event.issue.pull_request && startsWith(github.event.comment.body, '/ci-run-full-tests') }} timeout-minutes: 5 steps: - # Bound to github.event.review.user.login (who actually submitted the - # review), not context.actor (whoever triggered this workflow *run*). - # Those differ if someone later hits "Re-run all jobs": actor becomes - # the re-runner while review.body/commit_id/user stay frozen from the - # original event, which would otherwise let the re-runner's - # permission stand in for the original reviewer's. - - name: Check reviewer has maintain/admin permission + - name: Resolve PR head sha + id: pr + uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + with: + script: | + const { data: pr } = await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: context.issue.number, + }); + core.setOutput('sha', pr.head.sha); + + # Bound to github.event.comment.user.login (who actually posted the + # comment), not context.actor (whoever triggered this workflow *run* + # — those differ on a manual re-run, where actor becomes the + # re-runner while the comment payload stays frozen from the + # original event). + - name: Check commenter has maintain/admin permission uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 env: - REVIEWER: ${{ github.event.review.user.login }} + COMMENTER: ${{ github.event.comment.user.login }} with: script: | - const reviewer = process.env.REVIEWER; + const commenter = process.env.COMMENTER; const { data } = await github.rest.repos.getCollaboratorPermissionLevel({ owner: context.repo.owner, repo: context.repo.repo, - username: reviewer, + username: commenter, }); - core.info(`${reviewer} has permission: ${data.permission}`); + core.info(`${commenter} has permission: ${data.permission}`); if (!['maintain', 'admin'].includes(data.permission)) { core.setFailed( - `@${reviewer} has '${data.permission}' permission on this repo, ` + + `@${commenter} has '${data.permission}' permission on this repo, ` + `but triggering the full test suite requires 'maintain' or 'admin'.` ); } @@ -63,8 +85,8 @@ jobs: - name: Set commit status to pending uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 env: - REVIEWER: ${{ github.event.review.user.login }} - SHA: ${{ github.event.review.commit_id }} + COMMENTER: ${{ github.event.comment.user.login }} + SHA: ${{ steps.pr.outputs.sha }} with: script: | await github.rest.repos.createCommitStatus({ @@ -73,21 +95,21 @@ jobs: sha: process.env.SHA, state: 'pending', context: 'full-tests / make test-all', - description: `Triggered by @${process.env.REVIEWER}`, - target_url: `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/pull/${{ github.event.pull_request.number }}`, + description: `Triggered by @${process.env.COMMENTER}`, + target_url: `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/pull/${context.issue.number}`, }) # Dispatch against `main` (a ref guaranteed to exist in this repo), # not the PR's own branch: for fork PRs, the head branch name only # exists in the fork, not here, and workflow_dispatch's `ref` must # name a branch/tag in *this* repo. full-tests.yml's own checkout - # step is what actually fetches `inputs.sha` — that's the exact - # commit under review, regardless of which ref ran the dispatch. + # step is what actually fetches `inputs.sha` — that's the PR head + # commit resolved above, regardless of which ref ran the dispatch. - name: Dispatch full-tests.yml uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 env: - SHA: ${{ github.event.review.commit_id }} - PR_NUMBER: ${{ github.event.pull_request.number }} + SHA: ${{ steps.pr.outputs.sha }} + PR_NUMBER: ${{ github.event.issue.number }} with: script: | await github.rest.actions.createWorkflowDispatch({ From 74225e9d21aba80212cf59131a610039032bdc35 Mon Sep 17 00:00:00 2001 From: David Yaffe Date: Mon, 17 Aug 2026 16:39:31 -0400 Subject: [PATCH 06/14] TEMPORARY: add pull_request trigger to validate the job graph runs green Only running the redesigned validate-inputs/set-pending/full-tests/ report-status split, the protoc fix, and the docker-compose service setup once so far, on a maintainer-comment trigger that can't fire until this merges to main. Adding pull_request: here to actually see it pass before switching the real trigger on. Doesn't post commit statuses on pull_request runs (gated to workflow_dispatch only) so this can't accidentally satisfy the same-named required check on other PRs in the repo. Remove this trigger and the two workflow_ dispatch-only conditions before merging. --- .github/workflows/full-tests.yml | 33 ++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/.github/workflows/full-tests.yml b/.github/workflows/full-tests.yml index 89ab32131eb..573c606544c 100644 --- a/.github/workflows/full-tests.yml +++ b/.github/workflows/full-tests.yml @@ -30,6 +30,11 @@ name: Full test suite # (`validate-inputs`) before any other job trusts them, and passed to # github-script via `env:`/`process.env` rather than templated into script # source, so a crafted input can't break out of a string literal. +# TEMPORARY: pull_request: added to validate the redesigned job graph +# (validate-inputs/set-pending/full-tests/report-status split, the protoc +# fix, docker-compose services) actually runs green before wiring up the +# real workflow_dispatch-only trigger. Remove this trigger, and the +# `|| github.event.pull_request...` fallbacks below, before merging. on: workflow_dispatch: inputs: @@ -41,6 +46,7 @@ on: description: "PR number (used only for the failure notification link)" required: true type: string + pull_request: permissions: contents: read @@ -49,7 +55,7 @@ env: STATUS_CONTEXT: "full-tests / make test-all" concurrency: - group: ${{ github.workflow }}-${{ inputs.sha }} + group: ${{ github.workflow }}-${{ inputs.sha || github.event.pull_request.head.sha }} cancel-in-progress: true jobs: @@ -61,8 +67,8 @@ jobs: steps: - name: Validate sha and pr_number are well-formed env: - SHA: ${{ inputs.sha }} - PR_NUMBER: ${{ inputs.pr_number }} + SHA: ${{ inputs.sha || github.event.pull_request.head.sha }} + PR_NUMBER: ${{ inputs.pr_number || github.event.pull_request.number }} run: | if ! [[ "$SHA" =~ ^[0-9a-fA-F]{40}$ ]]; then echo "::error::sha must be a 40-character hex commit SHA, got: $SHA" @@ -76,6 +82,11 @@ jobs: set-pending: name: Set pending status needs: [validate-inputs] + # TEMPORARY (see `on:` above): while pull_request: is active, don't + # post a real commit status on every PR push in the repo — that could + # auto-satisfy this exact check's context if it's already required + # elsewhere. Remove this condition along with the trigger before merge. + if: ${{ github.event_name == 'workflow_dispatch' }} runs-on: ubuntu-latest timeout-minutes: 2 permissions: @@ -83,7 +94,7 @@ jobs: steps: - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 env: - SHA: ${{ inputs.sha }} + SHA: ${{ inputs.sha || github.event.pull_request.head.sha }} with: script: | await github.rest.repos.createCommitStatus({ @@ -99,6 +110,10 @@ jobs: full-tests: name: All-features tests + failpoints (make test-all) needs: [set-pending] + # A skipped set-pending (see TEMPORARY note there) would otherwise + # cascade into skipping this job too, since `needs` defaults to + # requiring success, not skipped-or-success. + if: ${{ !failure() && !cancelled() }} runs-on: gh-ubuntu-arm64 timeout-minutes: 60 permissions: @@ -106,7 +121,7 @@ jobs: steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: - ref: ${{ inputs.sha }} + ref: ${{ inputs.sha || github.event.pull_request.head.sha }} persist-credentials: false - name: Install Ubuntu packages @@ -164,7 +179,9 @@ jobs: report-status: name: Report final status needs: [validate-inputs, full-tests] - if: ${{ always() && needs.validate-inputs.result == 'success' }} + # TEMPORARY (see `on:` above): only post a real status for a + # workflow_dispatch run, same reasoning as set-pending. + if: ${{ always() && needs.validate-inputs.result == 'success' && github.event_name == 'workflow_dispatch' }} runs-on: ubuntu-latest timeout-minutes: 2 permissions: @@ -172,7 +189,7 @@ jobs: steps: - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 env: - SHA: ${{ inputs.sha }} + SHA: ${{ inputs.sha || github.event.pull_request.head.sha }} RESULT: ${{ needs.full-tests.result }} with: script: | @@ -203,7 +220,7 @@ jobs: color: "#FF0000" title: "" description: | - ### ❌ [PR #${{ inputs.pr_number }}](https://github.com/${{ github.repository }}/pull/${{ inputs.pr_number }}) + ### ❌ [PR #${{ inputs.pr_number || github.event.pull_request.number }}](https://github.com/${{ github.repository }}/pull/${{ inputs.pr_number || github.event.pull_request.number }}) The full test suite (`make test-all`) failed. From 933341ae278f0e1e06f3920b5271b1352c097edc Mon Sep 17 00:00:00 2001 From: David Yaffe Date: Mon, 17 Aug 2026 16:59:31 -0400 Subject: [PATCH 07/14] Switch full-tests job to a standard GitHub-hosted runner gh-ubuntu-arm64 killed the job at ~8 minutes on two separate runs (7m45s and 7m35s), both mid-compile, both with a runner shutdown signal rather than a test failure or our own timeout-minutes. Matches how vector's CI (test.yml) runs everything on plain GitHub-hosted runners (ubuntu-24.04, ubuntu-24.04-8core for the heavy jobs) rather than a custom label. Starting with the standard tier since we can't confirm quickwit-oss has GitHub's larger-runner tier provisioned. --- .github/workflows/full-tests.yml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/.github/workflows/full-tests.yml b/.github/workflows/full-tests.yml index 573c606544c..dd516c66dde 100644 --- a/.github/workflows/full-tests.yml +++ b/.github/workflows/full-tests.yml @@ -114,7 +114,13 @@ jobs: # cascade into skipping this job too, since `needs` defaults to # requiring success, not skipped-or-success. if: ${{ !failure() && !cancelled() }} - runs-on: gh-ubuntu-arm64 + # gh-ubuntu-arm64 (used by coverage.yml too) reproducibly killed this + # job at ~8 minutes across two separate runs — not our timeout-minutes, + # something infra-side. Vector's CI (test.yml) uses plain GitHub-hosted + # runners throughout (ubuntu-24.04, ubuntu-24.04-8core for heavy jobs); + # matching that here with the standard tier first since we can't + # confirm quickwit-oss has GitHub's larger-runner tier provisioned. + runs-on: ubuntu-latest timeout-minutes: 60 permissions: contents: read From 66f7883d87483a55c8d1853ea6da2a408ad621e1 Mon Sep 17 00:00:00 2001 From: David Yaffe Date: Wed, 19 Aug 2026 14:10:09 -0400 Subject: [PATCH 08/14] Switch full-tests to a reusable workflow gated by review or merge group - full-tests-trigger.yml now listens on pull_request_review (submitted) and uses the immutable review.commit_id, so the suite can't drift to a later push. - full-tests.yml is now a reusable workflow (workflow_call) also triggered by merge_group, so merge-queue candidates run the full suite automatically. - validate-inputs distinguishes the review path (PR open + head matches the reviewed SHA) from the merge-group path. - Failure notification labels the target as PR #N or merge-group commit. - Use standard ubuntu-24.04 runners. --- .github/workflows/full-tests-trigger.yml | 132 ++++----------- .github/workflows/full-tests.yml | 196 ++++++++++++----------- 2 files changed, 132 insertions(+), 196 deletions(-) diff --git a/.github/workflows/full-tests-trigger.yml b/.github/workflows/full-tests-trigger.yml index ae50b7345f1..6b216f4f5d9 100644 --- a/.github/workflows/full-tests-trigger.yml +++ b/.github/workflows/full-tests-trigger.yml @@ -1,124 +1,56 @@ name: Full test suite trigger -# `full-tests.yml` ("full-tests / make test-all") is a required status check -# but never runs on its own — see the comment there for why. This workflow -# is the trigger: a PR comment whose body starts with `/ci-run-full-tests`, -# and if that commenter actually holds `maintain` or `admin` permission on -# this repo, this sets a pending status on the PR's current head commit and -# dispatches full-tests.yml against it. -# -# Uses issue_comment (a plain PR comment on the Conversation tab), not -# pull_request_review — a submitted review pins naturally to a specific -# commit (github.event.review.commit_id); a plain comment doesn't carry a -# commit sha at all, so this fetches the PR's current head sha itself. That -# means there's a small window between commenting and this job running -# where a new push could move the head sha out from under the comment — -# accepted as a UX tradeoff for "just comment normally" over requiring the -# Review-changes flow. -# -# Deliberately checks the real collaborator permission level via the API -# rather than `github.event.comment.author_association` — that field can -# only tell you OWNER/MEMBER/COLLABORATOR/etc, none of which distinguish -# "has write access" from "has maintain/admin access". As of 2026-08-17, -# quickwit-oss/quickwit has 57 collaborators with at least write access but -# only 24 with maintain/admin — author_association would have let all 57 -# trigger this, not just the intended 24. +# A maintainer can run the expensive full suite by submitting a PR review +# beginning with `/ci-run-full-tests`. Review events include the immutable +# commit SHA that the review covers, so the suite cannot drift to a later push. on: - issue_comment: - types: [created] + pull_request_review: + types: [submitted] permissions: contents: read - statuses: write - actions: write - -concurrency: - group: full-tests-trigger-${{ github.event.comment.id }} - cancel-in-progress: false jobs: - trigger: - name: Trigger full test suite - runs-on: ubuntu-latest - # github.event.issue.pull_request only exists when the comment is on a - # PR (issue_comment fires for both issues and PRs). - if: ${{ github.event.issue.pull_request && startsWith(github.event.comment.body, '/ci-run-full-tests') }} + authorize: + name: Authorize full test suite + if: ${{ startsWith(github.event.review.body, '/ci-run-full-tests') }} + runs-on: ubuntu-24.04 timeout-minutes: 5 + permissions: + contents: read steps: - - name: Resolve PR head sha - id: pr - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 - with: - script: | - const { data: pr } = await github.rest.pulls.get({ - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: context.issue.number, - }); - core.setOutput('sha', pr.head.sha); - - # Bound to github.event.comment.user.login (who actually posted the - # comment), not context.actor (whoever triggered this workflow *run* - # — those differ on a manual re-run, where actor becomes the - # re-runner while the comment payload stays frozen from the - # original event). - - name: Check commenter has maintain/admin permission + # Check the actual review author rather than github.actor: re-runs change + # the actor while retaining the original review event payload. + - name: Check reviewer has maintain/admin permission uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 env: - COMMENTER: ${{ github.event.comment.user.login }} + REVIEWER: ${{ github.event.review.user.login }} with: script: | - const commenter = process.env.COMMENTER; + const reviewer = process.env.REVIEWER; const { data } = await github.rest.repos.getCollaboratorPermissionLevel({ owner: context.repo.owner, repo: context.repo.repo, - username: commenter, + username: reviewer, }); - core.info(`${commenter} has permission: ${data.permission}`); + core.info(`${reviewer} has permission: ${data.permission}`); if (!['maintain', 'admin'].includes(data.permission)) { core.setFailed( - `@${commenter} has '${data.permission}' permission on this repo, ` + + `@${reviewer} has '${data.permission}' permission on this repo, ` + `but triggering the full test suite requires 'maintain' or 'admin'.` ); } - - name: Set commit status to pending - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 - env: - COMMENTER: ${{ github.event.comment.user.login }} - SHA: ${{ steps.pr.outputs.sha }} - with: - script: | - await github.rest.repos.createCommitStatus({ - owner: context.repo.owner, - repo: context.repo.repo, - sha: process.env.SHA, - state: 'pending', - context: 'full-tests / make test-all', - description: `Triggered by @${process.env.COMMENTER}`, - target_url: `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/pull/${context.issue.number}`, - }) - - # Dispatch against `main` (a ref guaranteed to exist in this repo), - # not the PR's own branch: for fork PRs, the head branch name only - # exists in the fork, not here, and workflow_dispatch's `ref` must - # name a branch/tag in *this* repo. full-tests.yml's own checkout - # step is what actually fetches `inputs.sha` — that's the PR head - # commit resolved above, regardless of which ref ran the dispatch. - - name: Dispatch full-tests.yml - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 - env: - SHA: ${{ steps.pr.outputs.sha }} - PR_NUMBER: ${{ github.event.issue.number }} - with: - script: | - await github.rest.actions.createWorkflowDispatch({ - owner: context.repo.owner, - repo: context.repo.repo, - workflow_id: 'full-tests.yml', - ref: 'main', - inputs: { - sha: process.env.SHA, - pr_number: process.env.PR_NUMBER, - }, - }) + full-tests: + name: Run full test suite + needs: authorize + uses: ./.github/workflows/full-tests.yml + permissions: + contents: read + pull-requests: read + statuses: write + with: + sha: ${{ github.event.review.commit_id }} + pr_number: ${{ github.event.pull_request.number }} + secrets: + DISCORD_WEBHOOK: ${{ secrets.DISCORD_WEBHOOK }} diff --git a/.github/workflows/full-tests.yml b/.github/workflows/full-tests.yml index dd516c66dde..246b1fb3865 100644 --- a/.github/workflows/full-tests.yml +++ b/.github/workflows/full-tests.yml @@ -1,52 +1,24 @@ name: Full test suite -# Runs the full test suite (`make test-all`: --all-features + failpoints) -# against every broker/backend service (Kafka, Pulsar, Azurite, fake GCS, -# Pub/Sub emulator, LocalStack, Postgres). -# -# This is a REQUIRED status check for merging, but it never runs on its -# own — it only runs via workflow_dispatch, triggered by -# full-tests-trigger.yml when a reviewer with `maintain`/`admin` permission -# on the repo submits a PR review containing `/ci-run-full-tests`. -# -# Why gate it at all instead of running automatically on every PR: only -# `ci.yml` (`cargo nextest --features=postgres,metrics`) and -# `datafusion-ci.yml` run automatically. None of the other optional -# features (kafka, pulsar, sqs, gcp-pubsub, azure, gcs, failpoints, ...) or -# broker-backed integration tests get compiled/exercised otherwise pre-merge -# — the only workflow that already runs them (`coverage.yml`) triggers on -# `push` to `main`, i.e. after merge. Running the full suite unconditionally -# on every PR push was the first design here, but it doesn't match how -# quickwit's team already runs the sibling `vector` repo's CI (broker/full -# suites there are merge-queue- or maintainer-comment-gated, never -# automatic-for-everyone) — this mirrors that instead. -# -# Job layout is deliberately split so that the only job which checks out -# and executes untrusted PR content (`full-tests`) never holds -# `statuses: write`. `inputs.sha`/`inputs.pr_number` are workflow_dispatch -# inputs — free-text fields anyone with plain repo write access can set to -# anything via the Actions UI/API, bypassing full-tests-trigger.yml's -# permission check entirely — so they're validated to a strict shape -# (`validate-inputs`) before any other job trusts them, and passed to -# github-script via `env:`/`process.env` rather than templated into script -# source, so a crafted input can't break out of a string literal. -# TEMPORARY: pull_request: added to validate the redesigned job graph -# (validate-inputs/set-pending/full-tests/report-status split, the protoc -# fix, docker-compose services) actually runs green before wiring up the -# real workflow_dispatch-only trigger. Remove this trigger, and the -# `|| github.event.pull_request...` fallbacks below, before merging. +# Runs `make test-all` (--all-features plus failpoints) against every +# broker/backend service. A maintainer's submitted PR review invokes this as a +# reusable workflow; merge-queue commits run it automatically. on: - workflow_dispatch: + workflow_call: inputs: sha: - description: "Commit SHA to check out and report the status against" + description: "Immutable PR head SHA to test" required: true type: string pr_number: - description: "PR number (used only for the failure notification link)" + description: "PR number whose head SHA is tested" required: true type: string - pull_request: + secrets: + DISCORD_WEBHOOK: + required: false + merge_group: + types: [checks_requested] permissions: contents: read @@ -55,46 +27,79 @@ env: STATUS_CONTEXT: "full-tests / make test-all" concurrency: - group: ${{ github.workflow }}-${{ inputs.sha || github.event.pull_request.head.sha }} + group: ${{ github.workflow }}-${{ inputs.pr_number || github.event.merge_group.head_sha }} cancel-in-progress: true jobs: validate-inputs: - name: Validate inputs - runs-on: ubuntu-latest + name: Validate target + runs-on: ubuntu-24.04 timeout-minutes: 2 - permissions: {} + permissions: + contents: read + pull-requests: read + outputs: + sha: ${{ steps.validate.outputs.sha }} + pr_number: ${{ steps.validate.outputs.pr_number }} + review_triggered: ${{ steps.validate.outputs.review_triggered }} steps: - - name: Validate sha and pr_number are well-formed + - name: Validate target SHA + id: validate + uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 env: - SHA: ${{ inputs.sha || github.event.pull_request.head.sha }} - PR_NUMBER: ${{ inputs.pr_number || github.event.pull_request.number }} - run: | - if ! [[ "$SHA" =~ ^[0-9a-fA-F]{40}$ ]]; then - echo "::error::sha must be a 40-character hex commit SHA, got: $SHA" - exit 1 - fi - if ! [[ "$PR_NUMBER" =~ ^[0-9]+$ ]]; then - echo "::error::pr_number must be numeric, got: $PR_NUMBER" - exit 1 - fi + REVIEW_TRIGGERED: ${{ inputs.sha != '' }} + SHA: ${{ inputs.sha || github.event.merge_group.head_sha }} + PR_NUMBER: ${{ inputs.pr_number }} + with: + script: | + const reviewTriggered = process.env.REVIEW_TRIGGERED === 'true'; + const sha = process.env.SHA; + const prNumber = process.env.PR_NUMBER; + + if (!/^[0-9a-fA-F]{40}$/.test(sha)) { + core.setFailed(`sha must be a 40-character hex commit SHA, got: ${sha}`); + return; + } + + if (reviewTriggered) { + if (!/^[0-9]+$/.test(prNumber)) { + core.setFailed(`pr_number must be numeric, got: ${prNumber}`); + return; + } + const { data: pr } = await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: Number(prNumber), + }); + if (pr.state !== 'open') { + core.setFailed(`PR #${prNumber} is ${pr.state}, not open.`); + return; + } + if (pr.head.sha.toLowerCase() !== sha.toLowerCase()) { + core.setFailed( + `PR #${prNumber} head is ${pr.head.sha}, not reviewed SHA ${sha}. ` + + 'Submit a new /ci-run-full-tests review for the current head.' + ); + return; + } + } + + core.setOutput('sha', sha); + core.setOutput('pr_number', reviewTriggered ? prNumber : ''); + core.setOutput('review_triggered', String(reviewTriggered)); set-pending: name: Set pending status - needs: [validate-inputs] - # TEMPORARY (see `on:` above): while pull_request: is active, don't - # post a real commit status on every PR push in the repo — that could - # auto-satisfy this exact check's context if it's already required - # elsewhere. Remove this condition along with the trigger before merge. - if: ${{ github.event_name == 'workflow_dispatch' }} - runs-on: ubuntu-latest + needs: validate-inputs + runs-on: ubuntu-24.04 timeout-minutes: 2 permissions: statuses: write steps: - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 env: - SHA: ${{ inputs.sha || github.event.pull_request.head.sha }} + SHA: ${{ needs.validate-inputs.outputs.sha }} + STATUS_CONTEXT: ${{ env.STATUS_CONTEXT }} with: script: | await github.rest.repos.createCommitStatus({ @@ -102,32 +107,24 @@ jobs: repo: context.repo.repo, sha: process.env.SHA, state: 'pending', - context: '${{ env.STATUS_CONTEXT }}', + context: process.env.STATUS_CONTEXT, description: 'Running full test suite...', target_url: `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`, - }) + }); full-tests: name: All-features tests + failpoints (make test-all) - needs: [set-pending] - # A skipped set-pending (see TEMPORARY note there) would otherwise - # cascade into skipping this job too, since `needs` defaults to - # requiring success, not skipped-or-success. - if: ${{ !failure() && !cancelled() }} - # gh-ubuntu-arm64 (used by coverage.yml too) reproducibly killed this - # job at ~8 minutes across two separate runs — not our timeout-minutes, - # something infra-side. Vector's CI (test.yml) uses plain GitHub-hosted - # runners throughout (ubuntu-24.04, ubuntu-24.04-8core for heavy jobs); - # matching that here with the standard tier first since we can't - # confirm quickwit-oss has GitHub's larger-runner tier provisioned. - runs-on: ubuntu-latest + needs: [validate-inputs, set-pending] + runs-on: ubuntu-24.04 timeout-minutes: 60 + # This is the only job that executes the reviewed/merge-group code. It + # deliberately has no secret or write permission. permissions: contents: read steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: - ref: ${{ inputs.sha || github.event.pull_request.head.sha }} + ref: ${{ needs.validate-inputs.outputs.sha }} persist-credentials: false - name: Install Ubuntu packages @@ -135,10 +132,8 @@ jobs: sudo apt-get update sudo apt-get -y install libsasl2-dev libcurl4-openssl-dev - # apt's protobuf-compiler is too old to support proto3 optional fields - # by default, which the `substrait` crate (pulled in by --all-features - # via the `datafusion` feature) requires. coverage.yml hits the same - # constraint and installs protoc this way for the same reason. + # apt's protobuf-compiler is too old for proto3 optional fields required + # by the substrait crate enabled through the datafusion feature. - name: Install protoc uses: taiki-e/install-action@7769b73c2ec98c38dfcf2e18c83cfd4880c038c1 with: @@ -185,18 +180,17 @@ jobs: report-status: name: Report final status needs: [validate-inputs, full-tests] - # TEMPORARY (see `on:` above): only post a real status for a - # workflow_dispatch run, same reasoning as set-pending. - if: ${{ always() && needs.validate-inputs.result == 'success' && github.event_name == 'workflow_dispatch' }} - runs-on: ubuntu-latest + if: ${{ always() && needs.validate-inputs.result == 'success' }} + runs-on: ubuntu-24.04 timeout-minutes: 2 permissions: statuses: write steps: - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 env: - SHA: ${{ inputs.sha || github.event.pull_request.head.sha }} + SHA: ${{ needs.validate-inputs.outputs.sha }} RESULT: ${{ needs.full-tests.result }} + STATUS_CONTEXT: ${{ env.STATUS_CONTEXT }} with: script: | const state = process.env.RESULT === 'success' ? 'success' : 'failure'; @@ -205,28 +199,38 @@ jobs: repo: context.repo.repo, sha: process.env.SHA, state, - context: '${{ env.STATUS_CONTEXT }}', + context: process.env.STATUS_CONTEXT, description: state === 'success' ? 'Full test suite passed' : 'Full test suite failed', target_url: `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`, - }) + }); on-failure: - if: ${{ github.repository_owner == 'quickwit-oss' && needs.full-tests.result == 'failure' }} - name: On Failure - needs: [full-tests] - runs-on: ubuntu-latest + name: Send failure notification + needs: [validate-inputs, full-tests] + if: >- + ${{ always() && github.repository_owner == 'quickwit-oss' && + needs.validate-inputs.result == 'success' && needs.full-tests.result == 'failure' }} + runs-on: ubuntu-24.04 timeout-minutes: 2 permissions: {} + env: + TARGET: >- + ${{ needs.validate-inputs.outputs.review_triggered == 'true' && + format('PR #{0}', needs.validate-inputs.outputs.pr_number) || + format('merge-group commit {0}', needs.validate-inputs.outputs.sha) }} steps: - - name: Send Message + - name: Send message + if: env.DISCORD_WEBHOOK != '' + env: + DISCORD_WEBHOOK: ${{ secrets.DISCORD_WEBHOOK }} uses: sarisia/actions-status-discord@eb045afee445dc055c18d3d90bd0f244fd062708 # v1.16.0 with: - webhook: ${{ secrets.DISCORD_WEBHOOK }} + webhook: ${{ env.DISCORD_WEBHOOK }} nodetail: true color: "#FF0000" title: "" description: | - ### ❌ [PR #${{ inputs.pr_number || github.event.pull_request.number }}](https://github.com/${{ github.repository }}/pull/${{ inputs.pr_number || github.event.pull_request.number }}) + ### ❌ ${{ env.TARGET }} The full test suite (`make test-all`) failed. From ae581a8359ae35b62c0aadf8be85974ad5cfd24a Mon Sep 17 00:00:00 2001 From: David Yaffe Date: Wed, 19 Aug 2026 14:14:28 -0400 Subject: [PATCH 09/14] Document the untrusted-checkout threat model in full-tests.yml CodeQL's actions/untrusted-checkout rule flags the checkout in the full-tests job. The construct is intentional and already mitigated by privilege isolation, so record the reasoning in-place rather than restructuring the job: no secrets reference, contents: read only, persist-credentials: false, and a ref that is always either a maintainer-reviewed commit or a merge-queue candidate. --- .github/workflows/full-tests.yml | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/.github/workflows/full-tests.yml b/.github/workflows/full-tests.yml index 246b1fb3865..23649c63662 100644 --- a/.github/workflows/full-tests.yml +++ b/.github/workflows/full-tests.yml @@ -117,8 +117,22 @@ jobs: needs: [validate-inputs, set-pending] runs-on: ubuntu-24.04 timeout-minutes: 60 - # This is the only job that executes the reviewed/merge-group code. It - # deliberately has no secret or write permission. + # This is the only job that checks out and executes PR-authored code, and + # it is deliberately non-privileged. CodeQL flags this checkout + # (actions/untrusted-checkout): that is expected here, and is mitigated by + # isolation rather than by avoiding the checkout, which would defeat the + # entire purpose of the job. Specifically: + # - no secrets: this job never references `secrets.*`. DISCORD_WEBHOOK is + # only read by `on-failure`, which has `permissions: {}` and does not + # check out code. + # - no write scopes: `contents: read` is the minimum `actions/checkout` + # needs. The jobs holding `statuses: write` (set-pending, + # report-status) never check out or run PR code. + # - `persist-credentials: false`, so the token is not left in .git/config + # for the untrusted build steps to pick up. + # - the ref is never arbitrary fork input: the review path tests the exact + # commit a maintain/admin reviewer approved, and the merge_group path + # tests a candidate the merge queue built. permissions: contents: read steps: From f1953fe8247040943824eab2332c4735c1ab9ecd Mon Sep 17 00:00:00 2001 From: David Yaffe Date: Wed, 19 Aug 2026 15:02:22 -0400 Subject: [PATCH 10/14] Gate the full test suite behind a fork-compatible comment trigger Switch the trigger from pull_request_review to issue_comment. Fork PR review runs get a read-only token with secrets withheld, so the status writes would 403 and the required check could never be satisfied for external contributors -- the people this gate most needs to cover. issue_comment is a default-branch event, so the workflow definition is never read from the PR and the run keeps the scopes it needs. Test the PR's merge commit rather than its head, so the result reflects the code as merged. The command carries no SHA and means 'test this PR now'; both SHAs are resolved once, up front, and the run is pinned to that snapshot. Poll until GitHub has computed mergeability, since merge_commit_sha is populated asynchronously and would otherwise point at a stale tree. Make the untrusted job's cache read-only. Because the caller is a default-branch event, a cache written by PR-authored code would land in the default-branch scope, where every other PR and every trusted workflow on main would read it. Also simplify: fold SHA resolution into the pending-status job (dropping validate-inputs and its now-dead input validation, since SHAs come from the API rather than from a comment), and stop re-declaring workflow-level env inside steps. Rename the status context to full-test-suite: it is a long-lived branch protection contract and should not carry a Makefile target name. --- .github/workflows/full-tests-trigger.yml | 106 +++++++++-- .github/workflows/full-tests.yml | 231 +++++++++++++---------- 2 files changed, 218 insertions(+), 119 deletions(-) diff --git a/.github/workflows/full-tests-trigger.yml b/.github/workflows/full-tests-trigger.yml index 6b216f4f5d9..e7bb18585f1 100644 --- a/.github/workflows/full-tests-trigger.yml +++ b/.github/workflows/full-tests-trigger.yml @@ -1,46 +1,117 @@ name: Full test suite trigger -# A maintainer can run the expensive full suite by submitting a PR review -# beginning with `/ci-run-full-tests`. Review events include the immutable -# commit SHA that the review covers, so the suite cannot drift to a later push. +# A maintainer runs the expensive full suite by commenting `/ci-run-all-tests` +# on a pull request. +# +# `issue_comment` is a trusted, default-branch event: the workflow definition is +# always read from the default branch (never from the PR), and the run keeps the +# token scopes and secrets it needs to publish a commit status even when the PR +# comes from a fork. +# +# `pull_request_review` cannot do this. GitHub hands fork-PR review runs a +# read-only token and withholds secrets, so `statuses: write` would 403 and the +# required check could never be satisfied for external contributors -- the exact +# people an open-source repo has to support. on: - pull_request_review: - types: [submitted] + issue_comment: + types: [created] permissions: contents: read jobs: authorize: - name: Authorize full test suite - if: ${{ startsWith(github.event.review.body, '/ci-run-full-tests') }} + name: Authorize and resolve target + if: >- + ${{ github.event.issue.pull_request != null && + startsWith(github.event.comment.body, '/ci-run-all-tests') }} runs-on: ubuntu-24.04 timeout-minutes: 5 permissions: contents: read + pull-requests: read + outputs: + head_sha: ${{ steps.resolve.outputs.head_sha }} + merge_sha: ${{ steps.resolve.outputs.merge_sha }} steps: - # Check the actual review author rather than github.actor: re-runs change - # the actor while retaining the original review event payload. - - name: Check reviewer has maintain/admin permission + # Read the comment author from the event payload rather than github.actor. + # Re-running a workflow replaces the actor while retaining the original + # comment payload, so trusting github.actor would let a maintainer's re-run + # launder authorization for someone else's command. + - name: Check commenter has maintain/admin permission uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 env: - REVIEWER: ${{ github.event.review.user.login }} + COMMENTER: ${{ github.event.comment.user.login }} with: script: | - const reviewer = process.env.REVIEWER; + const commenter = process.env.COMMENTER; const { data } = await github.rest.repos.getCollaboratorPermissionLevel({ owner: context.repo.owner, repo: context.repo.repo, - username: reviewer, + username: commenter, }); - core.info(`${reviewer} has permission: ${data.permission}`); + core.info(`${commenter} has permission: ${data.permission}`); if (!['maintain', 'admin'].includes(data.permission)) { core.setFailed( - `@${reviewer} has '${data.permission}' permission on this repo, ` + + `@${commenter} has '${data.permission}' permission on this repo, ` + `but triggering the full test suite requires 'maintain' or 'admin'.` ); } + # The command deliberately carries no SHA: it means "test this PR as it is + # right now". Both SHAs are resolved here so the run is pinned to one + # immutable snapshot rather than following a moving target for 25 minutes. + # + # GitHub computes the test merge commit asynchronously, so `mergeable` is + # null and `merge_commit_sha` can be stale immediately after a push. Poll + # until mergeability is known instead of testing the wrong tree. + - name: Resolve PR head and merge commit + id: resolve + uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + with: + script: | + const pull_number = context.issue.number; + let pr; + for (let attempt = 1; attempt <= 10; attempt += 1) { + ({ data: pr } = await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number, + })); + if (pr.state !== 'open') { + core.setFailed(`PR #${pull_number} is ${pr.state}, not open.`); + return; + } + if (pr.mergeable !== null) { + break; + } + core.info(`Mergeability not computed yet (attempt ${attempt}/10); retrying in 3s.`); + await new Promise((resolve) => { setTimeout(resolve, 3000); }); + } + + if (pr.mergeable === null) { + core.setFailed( + `GitHub did not finish computing mergeability for PR #${pull_number}. ` + + 'Comment /ci-run-all-tests again in a moment.' + ); + return; + } + if (pr.mergeable === false) { + core.setFailed( + `PR #${pull_number} conflicts with ${pr.base.ref}. Merge or rebase ` + + `${pr.base.ref} before running the full suite.` + ); + return; + } + if (!pr.merge_commit_sha) { + core.setFailed(`PR #${pull_number} has no test merge commit to test.`); + return; + } + + core.info(`Testing merge commit ${pr.merge_commit_sha} (head ${pr.head.sha}).`); + core.setOutput('head_sha', pr.head.sha); + core.setOutput('merge_sha', pr.merge_commit_sha); + full-tests: name: Run full test suite needs: authorize @@ -50,7 +121,8 @@ jobs: pull-requests: read statuses: write with: - sha: ${{ github.event.review.commit_id }} - pr_number: ${{ github.event.pull_request.number }} + head_sha: ${{ needs.authorize.outputs.head_sha }} + merge_sha: ${{ needs.authorize.outputs.merge_sha }} + pr_number: ${{ github.event.issue.number }} secrets: DISCORD_WEBHOOK: ${{ secrets.DISCORD_WEBHOOK }} diff --git a/.github/workflows/full-tests.yml b/.github/workflows/full-tests.yml index 23649c63662..bbf077b1c53 100644 --- a/.github/workflows/full-tests.yml +++ b/.github/workflows/full-tests.yml @@ -1,17 +1,27 @@ name: Full test suite # Runs `make test-all` (--all-features plus failpoints) against every -# broker/backend service. A maintainer's submitted PR review invokes this as a -# reusable workflow; merge-queue commits run it automatically. +# broker/backend service. +# +# Two entry points: +# - full-tests-trigger.yml calls this as a reusable workflow when a maintainer +# comments `/ci-run-all-tests` on a PR. It tests the PR's *merge* commit, so +# the result reflects the code as merged rather than the branch in isolation. +# - merge_group runs it directly on merge-queue candidates. That event is +# trusted infrastructure, so it needs no authorization hop. on: workflow_call: inputs: - sha: - description: "Immutable PR head SHA to test" + head_sha: + description: "PR head SHA at resolution time (used to detect drift)" + required: true + type: string + merge_sha: + description: "PR test merge commit SHA -- this is what gets tested" required: true type: string pr_number: - description: "PR number whose head SHA is tested" + description: "PR number being tested" required: true type: string secrets: @@ -24,121 +34,98 @@ permissions: contents: read env: - STATUS_CONTEXT: "full-tests / make test-all" + # This string is a long-lived contract with branch protection: renaming it + # breaks merges until the ruleset is updated in lockstep. It is deliberately + # decoupled from the Makefile target name so the implementation can change + # without touching repository settings. + STATUS_CONTEXT: "full-test-suite" concurrency: group: ${{ github.workflow }}-${{ inputs.pr_number || github.event.merge_group.head_sha }} cancel-in-progress: true jobs: - validate-inputs: - name: Validate target + # Resolves which commit this run targets and publishes the pending status. + # Doubles as the single source of truth for downstream jobs, which avoids a + # separate resolve job and keeps the two entry points from diverging. + prepare: + name: Resolve target and set pending status runs-on: ubuntu-24.04 - timeout-minutes: 2 + timeout-minutes: 3 permissions: - contents: read - pull-requests: read + statuses: write outputs: - sha: ${{ steps.validate.outputs.sha }} - pr_number: ${{ steps.validate.outputs.pr_number }} - review_triggered: ${{ steps.validate.outputs.review_triggered }} + test_sha: ${{ steps.resolve.outputs.test_sha }} + head_sha: ${{ steps.resolve.outputs.head_sha }} + pr_number: ${{ steps.resolve.outputs.pr_number }} steps: - - name: Validate target SHA - id: validate + - name: Resolve target and set pending + id: resolve uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 env: - REVIEW_TRIGGERED: ${{ inputs.sha != '' }} - SHA: ${{ inputs.sha || github.event.merge_group.head_sha }} + MERGE_SHA: ${{ inputs.merge_sha }} + HEAD_SHA: ${{ inputs.head_sha }} PR_NUMBER: ${{ inputs.pr_number }} + MERGE_GROUP_SHA: ${{ github.event.merge_group.head_sha }} with: script: | - const reviewTriggered = process.env.REVIEW_TRIGGERED === 'true'; - const sha = process.env.SHA; - const prNumber = process.env.PR_NUMBER; + // pr_number is only set on the comment-triggered path; merge-queue + // runs identify their candidate through the event payload instead. + const prNumber = process.env.PR_NUMBER || ''; + const headSha = process.env.HEAD_SHA || ''; + const testSha = prNumber ? process.env.MERGE_SHA : process.env.MERGE_GROUP_SHA; - if (!/^[0-9a-fA-F]{40}$/.test(sha)) { - core.setFailed(`sha must be a 40-character hex commit SHA, got: ${sha}`); + if (!testSha) { + core.setFailed('Could not determine which commit to test.'); return; } - if (reviewTriggered) { - if (!/^[0-9]+$/.test(prNumber)) { - core.setFailed(`pr_number must be numeric, got: ${prNumber}`); - return; - } - const { data: pr } = await github.rest.pulls.get({ + core.setOutput('test_sha', testSha); + core.setOutput('head_sha', headSha); + core.setOutput('pr_number', prNumber); + + // Publish to every commit the required check might be evaluated + // against. GitHub prefers the test merge commit when it carries a + // status and otherwise falls back to the head commit; covering both + // removes that ambiguity instead of betting on one reading. + const targets = [...new Set([testSha, headSha].filter(Boolean))]; + for (const sha of targets) { + await github.rest.repos.createCommitStatus({ owner: context.repo.owner, repo: context.repo.repo, - pull_number: Number(prNumber), + sha, + state: 'pending', + context: process.env.STATUS_CONTEXT, + description: 'Running full test suite...', + target_url: `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`, }); - if (pr.state !== 'open') { - core.setFailed(`PR #${prNumber} is ${pr.state}, not open.`); - return; - } - if (pr.head.sha.toLowerCase() !== sha.toLowerCase()) { - core.setFailed( - `PR #${prNumber} head is ${pr.head.sha}, not reviewed SHA ${sha}. ` + - 'Submit a new /ci-run-full-tests review for the current head.' - ); - return; - } } - core.setOutput('sha', sha); - core.setOutput('pr_number', reviewTriggered ? prNumber : ''); - core.setOutput('review_triggered', String(reviewTriggered)); - - set-pending: - name: Set pending status - needs: validate-inputs - runs-on: ubuntu-24.04 - timeout-minutes: 2 - permissions: - statuses: write - steps: - - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 - env: - SHA: ${{ needs.validate-inputs.outputs.sha }} - STATUS_CONTEXT: ${{ env.STATUS_CONTEXT }} - with: - script: | - await github.rest.repos.createCommitStatus({ - owner: context.repo.owner, - repo: context.repo.repo, - sha: process.env.SHA, - state: 'pending', - context: process.env.STATUS_CONTEXT, - description: 'Running full test suite...', - target_url: `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`, - }); - full-tests: name: All-features tests + failpoints (make test-all) - needs: [validate-inputs, set-pending] + needs: prepare runs-on: ubuntu-24.04 timeout-minutes: 60 - # This is the only job that checks out and executes PR-authored code, and - # it is deliberately non-privileged. CodeQL flags this checkout - # (actions/untrusted-checkout): that is expected here, and is mitigated by + # This is the only job that checks out and executes PR-authored code, and it + # is deliberately non-privileged. CodeQL flags this checkout + # (actions/untrusted-checkout); that is expected, and is mitigated by # isolation rather than by avoiding the checkout, which would defeat the # entire purpose of the job. Specifically: # - no secrets: this job never references `secrets.*`. DISCORD_WEBHOOK is - # only read by `on-failure`, which has `permissions: {}` and does not + # read only by `on-failure`, which has `permissions: {}` and does not # check out code. # - no write scopes: `contents: read` is the minimum `actions/checkout` - # needs. The jobs holding `statuses: write` (set-pending, - # report-status) never check out or run PR code. + # needs. The jobs holding `statuses: write` (prepare, report-status) + # never check out or run PR code. # - `persist-credentials: false`, so the token is not left in .git/config # for the untrusted build steps to pick up. - # - the ref is never arbitrary fork input: the review path tests the exact - # commit a maintain/admin reviewer approved, and the merge_group path - # tests a candidate the merge queue built. + # - the cache is read-only (see `save-if` below). permissions: contents: read steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: - ref: ${{ needs.validate-inputs.outputs.sha }} + ref: ${{ needs.prepare.outputs.test_sha }} persist-credentials: false - name: Install Ubuntu packages @@ -162,11 +149,18 @@ jobs: with: toolchain: stable - - name: Setup cache + # `save-if: false` is load-bearing, not an optimisation. The caller is a + # default-branch event, so this run's ref is the default branch and any + # cache this job wrote would land in the default-branch scope -- readable + # by every other PR and by trusted workflows on main. Reading a cache + # built by trusted runs is fine; letting PR-authored code write one is + # cache poisoning. Reads still warm the build, so the cost is negligible. + - name: Setup cache (read-only) uses: Swatinem/rust-cache@c19371144df3bb44fab255c43d04cbc2ab54d1c4 # v2.9.1 with: workspaces: "./quickwit -> target" shared-key: "quickwit-cargo-full" + save-if: false - name: Install cargo-nextest uses: taiki-e/install-action@7769b73c2ec98c38dfcf2e18c83cfd4880c038c1 @@ -193,50 +187,83 @@ jobs: report-status: name: Report final status - needs: [validate-inputs, full-tests] - if: ${{ always() && needs.validate-inputs.result == 'success' }} + needs: [prepare, full-tests] + if: ${{ always() && needs.prepare.result == 'success' }} runs-on: ubuntu-24.04 - timeout-minutes: 2 + timeout-minutes: 3 permissions: statuses: write + pull-requests: read steps: - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 env: - SHA: ${{ needs.validate-inputs.outputs.sha }} + TEST_SHA: ${{ needs.prepare.outputs.test_sha }} + HEAD_SHA: ${{ needs.prepare.outputs.head_sha }} + PR_NUMBER: ${{ needs.prepare.outputs.pr_number }} RESULT: ${{ needs.full-tests.result }} - STATUS_CONTEXT: ${{ env.STATUS_CONTEXT }} with: script: | const state = process.env.RESULT === 'success' ? 'success' : 'failure'; - await github.rest.repos.createCommitStatus({ - owner: context.repo.owner, - repo: context.repo.repo, - sha: process.env.SHA, - state, - context: process.env.STATUS_CONTEXT, - description: state === 'success' ? 'Full test suite passed' : 'Full test suite failed', - target_url: `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`, - }); + const prNumber = process.env.PR_NUMBER; + const testSha = process.env.TEST_SHA; + const headSha = process.env.HEAD_SHA; + + // Drift detection is informational only. The status is pinned to the + // SHAs actually tested, so a push (or a new commit on the base + // branch) produces a different merge commit that carries no status + // and leaves the required check unsatisfied. The gate therefore + // fails closed structurally; this block only explains why. + if (prNumber) { + const { data: pr } = await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: Number(prNumber), + }); + if (pr.head.sha !== headSha || pr.merge_commit_sha !== testSha) { + core.notice( + `PR #${prNumber} moved while the suite was running ` + + `(head ${headSha} -> ${pr.head.sha}, merge ${testSha} -> ${pr.merge_commit_sha}). ` + + 'This result applies only to the commits that were tested, so the ' + + 'required check stays unsatisfied until /ci-run-all-tests is run again.' + ); + } + } + + const targets = [...new Set([testSha, headSha].filter(Boolean))]; + for (const sha of targets) { + await github.rest.repos.createCommitStatus({ + owner: context.repo.owner, + repo: context.repo.repo, + sha, + state, + context: process.env.STATUS_CONTEXT, + description: state === 'success' ? 'Full test suite passed' : 'Full test suite failed', + target_url: `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`, + }); + } on-failure: name: Send failure notification - needs: [validate-inputs, full-tests] + needs: [prepare, full-tests] if: >- ${{ always() && github.repository_owner == 'quickwit-oss' && - needs.validate-inputs.result == 'success' && needs.full-tests.result == 'failure' }} + needs.prepare.result == 'success' && needs.full-tests.result == 'failure' }} runs-on: ubuntu-24.04 timeout-minutes: 2 permissions: {} + # Declared at job level so the step's own `if` can see it: a step's inline + # `env` block is not reliably available when evaluating that same step's + # condition. This job never checks out code, so holding the webhook here is + # not an exposure to untrusted input. env: + DISCORD_WEBHOOK: ${{ secrets.DISCORD_WEBHOOK }} TARGET: >- - ${{ needs.validate-inputs.outputs.review_triggered == 'true' && - format('PR #{0}', needs.validate-inputs.outputs.pr_number) || - format('merge-group commit {0}', needs.validate-inputs.outputs.sha) }} + ${{ needs.prepare.outputs.pr_number != '' && + format('PR #{0}', needs.prepare.outputs.pr_number) || + format('merge-group commit {0}', needs.prepare.outputs.test_sha) }} steps: - name: Send message if: env.DISCORD_WEBHOOK != '' - env: - DISCORD_WEBHOOK: ${{ secrets.DISCORD_WEBHOOK }} uses: sarisia/actions-status-discord@eb045afee445dc055c18d3d90bd0f244fd062708 # v1.16.0 with: webhook: ${{ env.DISCORD_WEBHOOK }} From 40937d9303550e0dc0af1f43582a2aa81bb4aee3 Mon Sep 17 00:00:00 2001 From: David Yaffe Date: Wed, 19 Aug 2026 15:09:12 -0400 Subject: [PATCH 11/14] TEMPORARY: run full suite on pull_request to validate the job graph Also fixes a real bug in the authorization check: getCollaboratorPermissionLevel's `permission` field is coarse and reports a `maintain` role as `write`, so comparing it against 'maintain' admitted admins only. Threshold is now write access or above, stated explicitly. --- .github/workflows/full-tests-trigger.yml | 14 +++++++++---- .github/workflows/full-tests.yml | 25 ++++++++++++++++++++---- 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/.github/workflows/full-tests-trigger.yml b/.github/workflows/full-tests-trigger.yml index e7bb18585f1..d1e8aa9fd4f 100644 --- a/.github/workflows/full-tests-trigger.yml +++ b/.github/workflows/full-tests-trigger.yml @@ -50,11 +50,17 @@ jobs: repo: context.repo.repo, username: commenter, }); - core.info(`${commenter} has permission: ${data.permission}`); - if (!['maintain', 'admin'].includes(data.permission)) { + core.info(`${commenter}: permission=${data.permission} role=${data.role_name}`); + + // `permission` is the coarse field and only ever returns admin, + // write, read or none -- a `maintain` role reports here as `write`. + // Comparing it against 'maintain' therefore admits admins only, + // which is not the intent. Use `role_name` instead if a threshold + // above write access is ever wanted. + if (!['admin', 'write'].includes(data.permission)) { core.setFailed( - `@${commenter} has '${data.permission}' permission on this repo, ` + - `but triggering the full test suite requires 'maintain' or 'admin'.` + `@${commenter} has '${data.role_name}' access to this repo, but ` + + 'triggering the full test suite requires write access or above.' ); } diff --git a/.github/workflows/full-tests.yml b/.github/workflows/full-tests.yml index bbf077b1c53..eadd441418c 100644 --- a/.github/workflows/full-tests.yml +++ b/.github/workflows/full-tests.yml @@ -29,6 +29,17 @@ on: required: false merge_group: types: [checks_requested] + # TEMPORARY -- DELETE BEFORE MERGE, along with the three `||` fallbacks in the + # `prepare` job and the one in `concurrency` below. + # + # issue_comment workflows only ever run from the default branch, so the real + # trigger cannot be exercised from this PR. Running automatically on pushes + # here proves out the expensive job, the dual-SHA status writes and the + # report-status drift check. It does NOT exercise full-tests-trigger.yml's + # authorize job (permission check + mergeability polling), which stays + # unverified until this lands on main. + pull_request: + branches: [main] permissions: contents: read @@ -41,7 +52,9 @@ env: STATUS_CONTEXT: "full-test-suite" concurrency: - group: ${{ github.workflow }}-${{ inputs.pr_number || github.event.merge_group.head_sha }} + group: >- + ${{ github.workflow }}-${{ inputs.pr_number || + github.event.pull_request.number || github.event.merge_group.head_sha }} cancel-in-progress: true jobs: @@ -63,9 +76,13 @@ jobs: id: resolve uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 env: - MERGE_SHA: ${{ inputs.merge_sha }} - HEAD_SHA: ${{ inputs.head_sha }} - PR_NUMBER: ${{ inputs.pr_number }} + # The `|| github.event.pull_request.*` halves are TEMPORARY and exist + # only for the pull_request trigger above. On a pull_request run + # github.sha is the test merge commit, which is exactly what the + # comment path passes as merge_sha. + MERGE_SHA: ${{ inputs.merge_sha || github.sha }} + HEAD_SHA: ${{ inputs.head_sha || github.event.pull_request.head.sha }} + PR_NUMBER: ${{ inputs.pr_number || github.event.pull_request.number }} MERGE_GROUP_SHA: ${{ github.event.merge_group.head_sha }} with: script: | From 21ff464b9a69cc8fb83b1bbafe70c3a4f20b3ad1 Mon Sep 17 00:00:00 2001 From: David Yaffe Date: Wed, 19 Aug 2026 16:01:57 -0400 Subject: [PATCH 12/14] Clean up the full-test gate: single status, warm cache, no scaffolding Three changes: - Report the status on the PR head only, not on both the head and the merge commit. Every other required check here is a check run on the head, so that is where branch protection looks; the merge-commit status was ignored for gating and only produced a duplicate entry in the PR check list. The tested merge commit is now named in the status description instead. - Read the `quickwit-cargo` cache rather than a dedicated `quickwit-cargo-full` key. With `save-if: false` nothing would ever have written a private key, so every run would have started cold. ci.yml populates `quickwit-cargo` from pushes to main on the same x64 runners (cache keys are arch-scoped) and its lints job builds --all-features. - Drop the temporary pull_request trigger and its fallbacks now that the job graph is proven: 3022 tests + 10 failpoints tests green. --- .github/workflows/full-tests.yml | 138 ++++++++++++++++--------------- 1 file changed, 72 insertions(+), 66 deletions(-) diff --git a/.github/workflows/full-tests.yml b/.github/workflows/full-tests.yml index eadd441418c..de7084ab798 100644 --- a/.github/workflows/full-tests.yml +++ b/.github/workflows/full-tests.yml @@ -5,15 +5,16 @@ name: Full test suite # # Two entry points: # - full-tests-trigger.yml calls this as a reusable workflow when a maintainer -# comments `/ci-run-all-tests` on a PR. It tests the PR's *merge* commit, so -# the result reflects the code as merged rather than the branch in isolation. +# comments `/ci-run-all-tests` on a PR. It tests the PR's *merge* commit so +# the result reflects the code as merged rather than the branch in isolation, +# but reports the status on the PR *head* (see `prepare`). # - merge_group runs it directly on merge-queue candidates. That event is # trusted infrastructure, so it needs no authorization hop. on: workflow_call: inputs: head_sha: - description: "PR head SHA at resolution time (used to detect drift)" + description: "PR head SHA -- the commit the status is reported on" required: true type: string merge_sha: @@ -29,17 +30,6 @@ on: required: false merge_group: types: [checks_requested] - # TEMPORARY -- DELETE BEFORE MERGE, along with the three `||` fallbacks in the - # `prepare` job and the one in `concurrency` below. - # - # issue_comment workflows only ever run from the default branch, so the real - # trigger cannot be exercised from this PR. Running automatically on pushes - # here proves out the expensive job, the dual-SHA status writes and the - # report-status drift check. It does NOT exercise full-tests-trigger.yml's - # authorize job (permission check + mergeability polling), which stays - # unverified until this lands on main. - pull_request: - branches: [main] permissions: contents: read @@ -52,9 +42,7 @@ env: STATUS_CONTEXT: "full-test-suite" concurrency: - group: >- - ${{ github.workflow }}-${{ inputs.pr_number || - github.event.pull_request.number || github.event.merge_group.head_sha }} + group: ${{ github.workflow }}-${{ inputs.pr_number || github.event.merge_group.head_sha }} cancel-in-progress: true jobs: @@ -69,20 +57,16 @@ jobs: statuses: write outputs: test_sha: ${{ steps.resolve.outputs.test_sha }} - head_sha: ${{ steps.resolve.outputs.head_sha }} + status_sha: ${{ steps.resolve.outputs.status_sha }} pr_number: ${{ steps.resolve.outputs.pr_number }} steps: - name: Resolve target and set pending id: resolve uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 env: - # The `|| github.event.pull_request.*` halves are TEMPORARY and exist - # only for the pull_request trigger above. On a pull_request run - # github.sha is the test merge commit, which is exactly what the - # comment path passes as merge_sha. - MERGE_SHA: ${{ inputs.merge_sha || github.sha }} - HEAD_SHA: ${{ inputs.head_sha || github.event.pull_request.head.sha }} - PR_NUMBER: ${{ inputs.pr_number || github.event.pull_request.number }} + MERGE_SHA: ${{ inputs.merge_sha }} + HEAD_SHA: ${{ inputs.head_sha }} + PR_NUMBER: ${{ inputs.pr_number }} MERGE_GROUP_SHA: ${{ github.event.merge_group.head_sha }} with: script: | @@ -97,26 +81,39 @@ jobs: return; } + // We test the merge commit but report on the PR head, and those are + // deliberately different commits. + // + // Every other required check here (CI / Lints, CI / Unit tests) is a + // check run on the head commit, so that is where branch protection + // looks. A status on the merge commit is therefore ignored for + // gating and only shows up as a confusing duplicate entry in the + // PR's check list. + // + // The tradeoff: because the status is pinned to the head, a later + // push invalidates it (new head, no status -> required check + // unsatisfied), but a new commit on the base branch does not, so a + // green result can reflect an older merge base. That is inherent to + // any "test the PR" gate and is exactly what the merge_group path + // fixes, since the queue rebuilds the candidate against current main. + // + // On the merge-queue path there is no PR head, so the candidate + // commit is both tested and reported on. + const statusSha = headSha || testSha; + core.setOutput('test_sha', testSha); - core.setOutput('head_sha', headSha); + core.setOutput('status_sha', statusSha); core.setOutput('pr_number', prNumber); - // Publish to every commit the required check might be evaluated - // against. GitHub prefers the test merge commit when it carries a - // status and otherwise falls back to the head commit; covering both - // removes that ambiguity instead of betting on one reading. - const targets = [...new Set([testSha, headSha].filter(Boolean))]; - for (const sha of targets) { - await github.rest.repos.createCommitStatus({ - owner: context.repo.owner, - repo: context.repo.repo, - sha, - state: 'pending', - context: process.env.STATUS_CONTEXT, - description: 'Running full test suite...', - target_url: `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`, - }); - } + await github.rest.repos.createCommitStatus({ + owner: context.repo.owner, + repo: context.repo.repo, + sha: statusSha, + state: 'pending', + context: process.env.STATUS_CONTEXT, + description: 'Running full test suite...', + target_url: `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`, + }); full-tests: name: All-features tests + failpoints (make test-all) @@ -171,12 +168,19 @@ jobs: # cache this job wrote would land in the default-branch scope -- readable # by every other PR and by trusted workflows on main. Reading a cache # built by trusted runs is fine; letting PR-authored code write one is - # cache poisoning. Reads still warm the build, so the cost is negligible. + # cache poisoning. + # + # Since this job never saves, it must read a key some *trusted* workflow + # populates, hence `quickwit-cargo` rather than a key of its own: ci.yml + # runs on push to main on the same ubuntu x64 runners (cache keys are + # arch-scoped) and its `lints` job builds `--all-features`, so the + # expensive dependency artifacts are already there. A dedicated key would + # simply never be written by anyone and every run would start cold. - name: Setup cache (read-only) uses: Swatinem/rust-cache@c19371144df3bb44fab255c43d04cbc2ab54d1c4 # v2.9.1 with: workspaces: "./quickwit -> target" - shared-key: "quickwit-cargo-full" + shared-key: "quickwit-cargo" save-if: false - name: Install cargo-nextest @@ -215,7 +219,7 @@ jobs: - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 env: TEST_SHA: ${{ needs.prepare.outputs.test_sha }} - HEAD_SHA: ${{ needs.prepare.outputs.head_sha }} + STATUS_SHA: ${{ needs.prepare.outputs.status_sha }} PR_NUMBER: ${{ needs.prepare.outputs.pr_number }} RESULT: ${{ needs.full-tests.result }} with: @@ -223,41 +227,43 @@ jobs: const state = process.env.RESULT === 'success' ? 'success' : 'failure'; const prNumber = process.env.PR_NUMBER; const testSha = process.env.TEST_SHA; - const headSha = process.env.HEAD_SHA; + // On the comment path this is the PR head; on the merge-queue path + // it is the candidate commit. + const statusSha = process.env.STATUS_SHA; - // Drift detection is informational only. The status is pinned to the - // SHAs actually tested, so a push (or a new commit on the base - // branch) produces a different merge commit that carries no status - // and leaves the required check unsatisfied. The gate therefore - // fails closed structurally; this block only explains why. + // Drift detection is informational only. The status is pinned to an + // immutable SHA, so a push produces a new head that carries no + // status and leaves the required check unsatisfied. The gate fails + // closed structurally; this block only explains why in the log. if (prNumber) { const { data: pr } = await github.rest.pulls.get({ owner: context.repo.owner, repo: context.repo.repo, pull_number: Number(prNumber), }); - if (pr.head.sha !== headSha || pr.merge_commit_sha !== testSha) { + if (pr.head.sha !== statusSha || pr.merge_commit_sha !== testSha) { core.notice( `PR #${prNumber} moved while the suite was running ` + - `(head ${headSha} -> ${pr.head.sha}, merge ${testSha} -> ${pr.merge_commit_sha}). ` + - 'This result applies only to the commits that were tested, so the ' + + `(head ${statusSha} -> ${pr.head.sha}, merge ${testSha} -> ${pr.merge_commit_sha}). ` + + 'This result applies only to the commit that was tested, so the ' + 'required check stays unsatisfied until /ci-run-all-tests is run again.' ); } } - const targets = [...new Set([testSha, headSha].filter(Boolean))]; - for (const sha of targets) { - await github.rest.repos.createCommitStatus({ - owner: context.repo.owner, - repo: context.repo.repo, - sha, - state, - context: process.env.STATUS_CONTEXT, - description: state === 'success' ? 'Full test suite passed' : 'Full test suite failed', - target_url: `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`, - }); - } + // Name the tested commit in the description: the status hangs on the + // head, so it is otherwise invisible which merge result was proven. + const suffix = testSha === statusSha ? '' : ` (merge commit ${testSha.slice(0, 7)})`; + + await github.rest.repos.createCommitStatus({ + owner: context.repo.owner, + repo: context.repo.repo, + sha: statusSha, + state, + context: process.env.STATUS_CONTEXT, + description: (state === 'success' ? 'Full test suite passed' : 'Full test suite failed') + suffix, + target_url: `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`, + }); on-failure: name: Send failure notification From 31a889605ea4d588edacf1212e110cafe9370bdd Mon Sep 17 00:00:00 2001 From: David Yaffe Date: Wed, 19 Aug 2026 16:33:11 -0400 Subject: [PATCH 13/14] Correct the authorization step name to match the write-access threshold --- .github/workflows/full-tests-trigger.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/full-tests-trigger.yml b/.github/workflows/full-tests-trigger.yml index d1e8aa9fd4f..2068dd98d37 100644 --- a/.github/workflows/full-tests-trigger.yml +++ b/.github/workflows/full-tests-trigger.yml @@ -38,7 +38,7 @@ jobs: # Re-running a workflow replaces the actor while retaining the original # comment payload, so trusting github.actor would let a maintainer's re-run # launder authorization for someone else's command. - - name: Check commenter has maintain/admin permission + - name: Check commenter has write access or above uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 env: COMMENTER: ${{ github.event.comment.user.login }} From 66254dcd37a5abde0abee9204fae32d4beba11d5 Mon Sep 17 00:00:00 2001 From: David Yaffe Date: Wed, 19 Aug 2026 16:41:22 -0400 Subject: [PATCH 14/14] Document the full-test PR command --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ba4e68c831c..558ea09bb53 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -16,7 +16,7 @@ Feel free to send your contribution in an unfinished state to get early feedback In that case, simply mark the PR with the tag [WIP] (standing for work in progress). ## PR verification checks -When you submit a pull request to the project, the CI system runs several verification checks. After your PR is merged, a more exhaustive list of tests will be run. +When you submit a pull request to the project, the CI system runs several verification checks. A collaborator with write access or above can run the full test suite by commenting `/ci-run-all-tests` on the pull request. This runs `make test-all` (all features, failpoints, and all broker backends) against the pull request merged with its base branch and publishes the `full-test-suite` commit status. It takes approximately 22 minutes. If you push new commits afterwards, the command must be run again. External contributors should ask a maintainer to run it. You will be notified by email from the CI system if any issues are discovered, but if you want to run these checks locally before submitting PR or in order to verify changes you can use the following commands in the root directory: 1. To verify that all tests are passing, run `make test-all`.