chore(CI): Run the full test suite automatically on every PR commit - #6782
chore(CI): Run the full test suite automatically on every PR commit#6782dayaffe wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9dc54470ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pull_request: | ||
| types: [opened, synchronize, reopened] |
There was a problem hiding this comment.
For fork PRs, the pull_request event executes this existing workflow from the PR's merge revision, so an author can modify full-tests.yml, retain the workflow/job names, and replace or remove the make test-all step; the resulting successful check can then satisfy the required check without running the suite. The deleted issue_comment dispatcher avoided this because it executed the trusted default-branch workflow, so preserve an equivalent trusted dispatcher or otherwise ensure the required workflow definition cannot be supplied by the PR.
AGENTS.md reference: AGENTS.md:L20-L20
Useful? React with 👍 / 👎.
| per_page: 100, | ||
| page, | ||
| }); |
There was a problem hiding this comment.
When a PR contains more than 3,000 changed files, GitHub's List pull request files endpoint exposes at most the first 3,000, but this pagination loop treats the end of that truncated result as a complete list. If the returned paths are all documentation or UI files while an omitted path affects the Rust workspace or build, affects_suite becomes false and the required check succeeds without running the suite; compare the number inspected with the PR's changed_files count or conservatively run the suite when the API cap is reached.
AGENTS.md reference: AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
| DISCORD_WEBHOOK: | ||
| required: false | ||
| pull_request: | ||
| types: [opened, synchronize, reopened] |
There was a problem hiding this comment.
When an open PR is retargeted after this check passes, GitHub emits the pull_request activity type edited with a base-branch change rather than opened, synchronize, or reopened. This workflow therefore does not test the merge against the newly selected base, leaving the required result associated with the same head commit from the old target until another push or reopen occurs; include edited and gate it on github.event.changes.base so retargeting reruns the suite.
Useful? React with 👍 / 👎.
The `/ci-run-all-tests` trigger relied on maintainers remembering to run the suite, and nothing in branch protection enforced it. Every PR push now completes a tiny trigger workflow that wakes a trusted dispatcher on the default branch, which runs `make test-all` against the PR merge commit, skips docs- and UI-only changes, and publishes the `full-test-suite` commit status that PR authors cannot forge.
9dc5447 to
99603c1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99603c1a7a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| jobs: | ||
| prepare: | ||
| name: Set pending status | ||
| if: github.event.workflow_run.conclusion != 'cancelled' |
There was a problem hiding this comment.
When a rapid push cancels the preceding run in full-tests-trigger.yml, both the cancelled trigger and the new successful trigger emit workflow_run: completed events. Both downstream runs enter the same branch-based concurrency group, and concurrency ordering is not guaranteed, so the cancelled-trigger dispatcher can start last and cancel the valid full-test run; this job-level condition then skips all work, leaving the newest commit's required status absent or permanently pending. Avoid launching cancelled-trigger dispatchers into the shared concurrency group, or give them a group that cannot cancel an active suite.
Useful? React with 👍 / 👎.
| with: | ||
| workspaces: "./quickwit -> target" | ||
| shared-key: "quickwit-cargo" | ||
| save-if: false |
There was a problem hiding this comment.
On fork PRs, make test-all executes arbitrary PR-authored code in this workflow_run job, whose Actions cache scope is the default branch. save-if: false only stops this particular action's post-step save; it does not withhold the runner's cache service URL and runtime token from the untrusted process, which can use them directly to publish a crafted quickwit-cargo cache. Subsequent trusted workflows restore that shared cache, allowing poisoned build artifacts or fingerprints to manipulate their tests, so this job must run in a PR-isolated cache context or otherwise prevent untrusted code from reaching the cache credentials.
AGENTS.md reference: AGENTS.md:L20-L20
Useful? React with 👍 / 👎.
| head: `${process.env.HEAD_OWNER}:${process.env.HEAD_BRANCH}`, | ||
| state: 'open', | ||
| }); | ||
| const pull = prs.find((pr) => pr.head.sha === process.env.HEAD_SHA); |
There was a problem hiding this comment.
When one source branch has concurrent open PRs targeting different base branches, pulls.list returns both with the same owner, branch, and head SHA, so this .find silently selects whichever entry appears first. Both dispatcher runs can consequently test the same PR merge commit and publish success to the shared head commit, leaving the other PR's merge against its own base untested; carry the triggering PR number into this workflow and reject ambiguous resolution instead of selecting the first match.
AGENTS.md reference: AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
| merge_sha: ${{ steps.resolve.outputs.merge_sha }} | ||
| affects_suite: ${{ steps.changes.outputs.affects_suite }} | ||
| steps: | ||
| # Resolve the PR by head and a fresh merge SHA. Mergeability is computed |
There was a problem hiding this comment.
@codex would it be cleaner to enable merge queue and make this a required check?
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99603c1a7a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pull_request: | ||
| types: [opened, synchronize, reopened] |
There was a problem hiding this comment.
When the repository's fork-workflow approval policy applies to a first-time or outside contributor, this pull_request workflow remains awaiting maintainer approval, so the downstream workflow_run never starts and the required full-test-suite status is absent. This contradicts the stated behavior of running automatically for every author and every pushed commit; use a trusted no-op pull_request_target dispatcher with minimal permissions, or otherwise account for the approval gate.
Useful? React with 👍 / 👎.
| pull_request: | ||
| types: [opened, synchronize, reopened] |
There was a problem hiding this comment.
If this check is made required for a merge queue as proposed, a queued PR creates a merge_group event rather than one of these pull_request events, so this dispatcher never starts and the queue waits for a full-test-suite status that cannot appear on the merge-group commit. Supporting that setup requires a merge_group trigger and a downstream path that tests and reports against the generated merge-group SHA instead of resolving it as a PR head.
Useful? React with 👍 / 👎.
2 participants
Description
Runs make test-all on every commit pushed to any PR, from any author including forks. The only exception is a commit that touches nothing but markdown, docs/, or quickwit/quickwit-ui/, which is skipped. Everything else, including dependency bumps and any file type it doesn't recognize, runs the full suite.
This will be a required check.