From 5dcac3f25981afe8ed2cceacac435bd5f13b8e32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Sun, 16 Aug 2026 20:10:03 +0100 Subject: [PATCH] ci: prepare CI workflows for a GitHub merge queue (#2721) Co-authored-by: Kandev Agent --- .github/scripts/changed-paths.py | 208 ++++++++++ .github/scripts/changed-paths_test.py | 355 ++++++++++++++++++ .../frontend-tests-workflow-contract_test.py | 69 ++-- .../scripts/release-workflow-contract_test.py | 39 +- .github/workflows/architecture-lint.yml | 49 ++- .github/workflows/backend-tests.yml | 183 +++++++-- .github/workflows/e2e-tests.yml | 182 +++++++-- .github/workflows/frontend-tests.yml | 197 ++++++++-- .github/workflows/lint-action-pinning.yml | 81 +--- .github/workflows/lint-harness-files.yml | 74 +--- apps/web/scripts/lib/e2e-sleep-wiring.test.ts | 9 +- docs/ci-merge-queue.md | 204 ++++++++++ 12 files changed, 1372 insertions(+), 278 deletions(-) create mode 100644 .github/scripts/changed-paths.py create mode 100644 .github/scripts/changed-paths_test.py create mode 100644 docs/ci-merge-queue.md diff --git a/.github/scripts/changed-paths.py b/.github/scripts/changed-paths.py new file mode 100644 index 0000000000..273c4c3272 --- /dev/null +++ b/.github/scripts/changed-paths.py @@ -0,0 +1,208 @@ +#!/usr/bin/env python3 +"""Apply a GitHub `paths:` filter inside a job instead of on the trigger. + +Why this exists +--------------- +A workflow that is skipped by a trigger-level `paths:` filter reports **no +conclusion at all**. GitHub's own documentation is explicit that "checks +associated with that workflow will remain in a 'Pending' state", and a required +check that stays pending blocks a pull request from entering a merge queue and +blocks the merge group from ever merging. + +The `merge_group` event makes this worse: it supports `types:` and `branches:` +filters only -- `paths:` is not available -- so a trigger-level filter cannot +even be mirrored onto the queue side. + +The fix is to move the filter one level down. The workflow triggers +unconditionally, a cheap `changes` job runs this script to decide whether the +expensive jobs are relevant, and an always-running gate job reports the single +conclusion the merge queue waits on. Jobs skipped by an `if:` conditional are +fine here because they are never themselves the required check. + +Usage +----- + git diff --name-only -z "$BASE" HEAD | python3 .github/scripts/changed-paths.py + +Reads the pattern list from `$PATTERNS` (override with `--patterns-env`), one +pattern per line, in the same syntax and with the same ordering semantics as a +workflow's `paths:` list. Writes `run=true` or `run=false` to stdout and, when +`$GITHUB_OUTPUT` is set, appends it there as a step output. + +Pass `-z` to the diff. Git quotes any path outside ASCII under its default +`core.quotePath=true`, so plain `--name-only` reports `apps/web/café.ts` as the +literal 24-character string `"apps/web/caf\303\251.ts"`. That matches no +pattern, so a pull request touching only such a file selects nothing, every job +is skipped, and the gate reports success having tested none of it -- the exact +silent pass this whole mechanism exists to prevent. `-z` emits raw bytes with a +NUL after each path and suppresses the quoting. + +Stdin is split on NUL when it contains one and on newlines otherwise, so both +forms work. The detection is not ambiguous: `-z` terminates every path with a +NUL, so any non-empty `-z` output contains at least one. + +Pattern syntax +-------------- +Supported: `*` (any run of characters except `/`), `**` (any run including +`/`), `?` (one character except `/`), and a leading `!` for negation. A file's +verdict is decided by the LAST pattern that matches it, which is what GitHub +does, so `!**/*.md` after `apps/web/**` excludes Markdown under `apps/web`. + +Deliberately unsupported: `+`, `[...]` character classes, and `{...}` braces. +GitHub accepts the first two; this script rejects any pattern containing them +rather than silently matching them as literals, because a filter that quietly +stops matching is exactly the failure this file exists to prevent. +""" + +from __future__ import annotations + +import argparse +import os +import re +import sys + + +# Characters GitHub's filter-pattern syntax gives a meaning that this script +# does not implement. Rejected loudly rather than treated as literals. +UNSUPPORTED_METACHARACTERS = ("+", "[", "]", "{", "}") + + +def compile_pattern(pattern: str) -> re.Pattern[str]: + """Translate one GitHub filter pattern into an anchored regex. + + `**` is translated before `*` so that the single-segment wildcard never + consumes half of a double one. + """ + parts: list[str] = [] + index = 0 + while index < len(pattern): + char = pattern[index] + if char == "*": + if pattern.startswith("**", index): + parts.append(".*") + index += 2 + else: + parts.append("[^/]*") + index += 1 + elif char == "?": + parts.append("[^/]") + index += 1 + else: + parts.append(re.escape(char)) + index += 1 + return re.compile("".join(parts) + r"\Z") + + +class Filter: + """An ordered `paths:` list, evaluated last-match-wins per file.""" + + def __init__(self, patterns: list[str]) -> None: + self.rules: list[tuple[bool, re.Pattern[str]]] = [] + for raw in patterns: + pattern = raw.strip() + # Blank lines and `#` comments keep the YAML block readable. + if not pattern or pattern.startswith("#"): + continue + + included = True + if pattern.startswith("!"): + included = False + pattern = pattern[1:] + + # A bare `!` compiles to a regex matching only the empty string, + # which no real path is. It would append a rule that can never fire + # and read like an exclusion that works. + if not pattern: + raise ValueError( + f"pattern {raw!r} is a bare negation with no path. Use " + "'!path/pattern' to exclude specific files." + ) + + found = [c for c in UNSUPPORTED_METACHARACTERS if c in pattern] + if found: + raise ValueError( + f"pattern {raw!r} uses unsupported metacharacter(s) " + f"{''.join(found)!r}. changed-paths.py implements *, ** and ? " + "only; extend it (and its tests) before using more." + ) + + self.rules.append((included, compile_pattern(pattern))) + + if not self.rules: + raise ValueError("no patterns were supplied") + + def includes(self, path: str) -> bool: + """Whether `path` survives the filter. Later patterns win.""" + verdict = False + for included, regex in self.rules: + if regex.match(path): + verdict = included + return verdict + + def matches_any(self, paths: list[str]) -> bool: + return any(self.includes(path) for path in paths) + + +def read_changed_paths(data: bytes) -> list[str]: + """Split a diff's path list, preferring the NUL-delimited form. + + Taken as bytes rather than text because `git diff -z` emits raw filesystem + bytes, which need not be valid UTF-8. `surrogateescape` keeps an + undecodable name intact and matchable instead of failing the step. + """ + separator = b"\0" if b"\0" in data else b"\n" + return [ + decoded + for chunk in data.split(separator) + if (decoded := chunk.decode("utf-8", errors="surrogateescape").strip()) + ] + + +def main(argv: list[str]) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--patterns-env", + default="PATTERNS", + help="environment variable holding the newline-separated pattern list", + ) + args = parser.parse_args(argv) + + raw_patterns = os.environ.get(args.patterns_env) + if raw_patterns is None: + print( + f"::error::{args.patterns_env} is unset; changed-paths.py has no " + "filter to apply.", + file=sys.stderr, + ) + return 2 + + try: + path_filter = Filter(raw_patterns.splitlines()) + except ValueError as error: + print(f"::error::{error}", file=sys.stderr) + return 2 + + changed = read_changed_paths(sys.stdin.buffer.read()) + run = path_filter.matches_any(changed) + + # The matching subset is the useful half of the log: "no relevant changes" + # is indistinguishable from "the filter is broken" without it. + if run: + print("Relevant changed files:") + for path in changed: + if path_filter.includes(path): + print(f" {path}") + else: + print(f"No relevant changes among {len(changed)} changed file(s).") + + value = "true" if run else "false" + github_output = os.environ.get("GITHUB_OUTPUT") + if github_output: + with open(github_output, "a", encoding="utf-8") as handle: + handle.write(f"run={value}\n") + + print(f"run={value}") + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:])) diff --git a/.github/scripts/changed-paths_test.py b/.github/scripts/changed-paths_test.py new file mode 100644 index 0000000000..cf4e7b6e7e --- /dev/null +++ b/.github/scripts/changed-paths_test.py @@ -0,0 +1,355 @@ +#!/usr/bin/env python3 +"""Tests for the in-job `paths:` filter used by the merge-queue gating jobs. + +Two halves. The first exercises the matcher directly. The second reads the +`PATTERNS` blocks out of the workflows that actually use it and asserts they +still select what their trigger-level `paths:` filters used to select -- a +regex that silently stops matching would hand every merge queue entry a full +E2E run, or worse, skip one that mattered. +""" + +from pathlib import Path +import os +import re +import subprocess +import sys +import tempfile +import unittest + +sys.path.insert(0, str(Path(__file__).resolve().parent)) + +from importlib import import_module + +changed_paths = import_module("changed-paths") + + +REPO_ROOT = Path(__file__).resolve().parents[2] +WORKFLOWS = REPO_ROOT / ".github" / "workflows" +SCRIPT = Path(__file__).resolve().parent / "changed-paths.py" + +# Every workflow that moved its filter into a `changes` job, with one path that +# must still select it and one that must not. +# Required checks with no `changes` job: cheap enough to always run, so their +# filtering was removed outright rather than moved. They still have to keep the +# merge_group trigger and stay unfiltered, and nothing else asserts it -- +# `lint-action-pinning.yml` is covered by the frontend and release contract +# tests, but these two had no guard at all. +ALWAYS_RUN_WORKFLOWS = ( + "architecture-lint.yml", + "lint-harness-files.yml", +) + +GATED_WORKFLOWS = { + "backend-tests.yml": ( + ["apps/backend/internal/orchestrator/orchestrator.go"], + ["apps/web/components/App.tsx", "apps/backend/AGENTS.md"], + ), + "frontend-tests.yml": ( + ["apps/web/components/App.tsx"], + ["apps/backend/internal/orchestrator/orchestrator.go", "apps/web/README.md"], + ), + "e2e-tests.yml": ( + ["apps/web/e2e/specs/kanban.spec.ts"], + ["docs/public/install.md", "README.md"], + ), +} + + +def patterns_block(workflow: str) -> str: + """Return the `PATTERNS:` heredoc-style YAML block of the `changes` job.""" + text = (WORKFLOWS / workflow).read_text(encoding="utf-8") + match = re.search(r"(?m)^ PATTERNS: \|\n((?:^ .*\n|^\n)+)", text) + if match is None: + raise AssertionError( + f"{workflow} has no `PATTERNS: |` block in its `changes` job. The " + "gating job cannot filter anything without one, so every job it " + "guards would run on every event -- or none would." + ) + return match.group(1) + + +class MatcherTest(unittest.TestCase): + def test_double_star_crosses_slashes_and_single_star_does_not(self) -> None: + crossing = changed_paths.Filter(["apps/web/**"]) + self.assertTrue(crossing.includes("apps/web/components/deep/App.tsx")) + + flat = changed_paths.Filter(["apps/web/*"]) + self.assertTrue(flat.includes("apps/web/package.json")) + self.assertFalse(flat.includes("apps/web/components/App.tsx")) + + def test_question_mark_matches_one_non_slash_character(self) -> None: + path_filter = changed_paths.Filter(["apps/?eb/main.ts"]) + self.assertTrue(path_filter.includes("apps/web/main.ts")) + self.assertFalse(path_filter.includes("apps//eb/main.ts")) + + def test_patterns_are_anchored_at_both_ends(self) -> None: + path_filter = changed_paths.Filter(["Makefile"]) + self.assertTrue(path_filter.includes("Makefile")) + self.assertFalse(path_filter.includes("apps/backend/Makefile")) + self.assertFalse(path_filter.includes("Makefile.bak")) + + def test_last_matching_pattern_decides(self) -> None: + """`!**/*.md` after an include is an exclusion, not an extra include.""" + path_filter = changed_paths.Filter(["apps/web/**", "!**/*.md"]) + + self.assertFalse(path_filter.includes("apps/web/README.md")) + self.assertTrue(path_filter.includes("apps/web/components/App.tsx")) + # Order matters: reversing the list makes the negation dead. + reversed_filter = changed_paths.Filter(["!**/*.md", "apps/web/**"]) + self.assertTrue(reversed_filter.includes("apps/web/README.md")) + + def test_a_single_relevant_file_selects_the_workflow(self) -> None: + path_filter = changed_paths.Filter(["apps/web/**", "!**/*.md"]) + + self.assertTrue( + path_filter.matches_any(["apps/web/README.md", "apps/web/lib/api.ts"]), + "A docs change bundled with a code change must still run the job.", + ) + self.assertFalse(path_filter.matches_any(["apps/web/README.md"])) + self.assertFalse( + path_filter.matches_any([]), + "An empty diff selects nothing; the caller decides whether an " + "unresolvable base means 'run everything'.", + ) + + def test_blank_lines_and_comments_are_ignored(self) -> None: + path_filter = changed_paths.Filter(["", "# a note", "apps/web/**", " "]) + self.assertTrue(path_filter.includes("apps/web/main.tsx")) + + def test_unsupported_metacharacters_are_rejected(self) -> None: + for pattern in ("apps/web/*.[jt]s", "apps/web/a+b", "apps/{web,cli}/**"): + with self.subTest(pattern=pattern): + with self.assertRaises(ValueError): + changed_paths.Filter([pattern]) + + def test_an_empty_pattern_list_is_rejected(self) -> None: + with self.assertRaises(ValueError): + changed_paths.Filter(["", "# only comments"]) + + def test_a_bare_negation_is_rejected(self) -> None: + """`!` alone compiles to a rule that can never match anything. + + It reads like an exclusion and silently is not one, so reject it the + same way an unsupported metacharacter is rejected. + """ + with self.assertRaises(ValueError): + changed_paths.Filter(["apps/web/**", "!"]) + + +class ChangedPathParsingTest(unittest.TestCase): + """`git diff -z` output, which is where the quoting trap lives.""" + + def test_nul_delimited_input_is_split_on_nul(self) -> None: + data = "apps/web/a.ts\0apps/web/b.ts\0".encode("utf-8") + self.assertEqual( + changed_paths.read_changed_paths(data), + ["apps/web/a.ts", "apps/web/b.ts"], + ) + + def test_newline_delimited_input_still_works(self) -> None: + data = b"apps/web/a.ts\napps/web/b.ts\n" + self.assertEqual( + changed_paths.read_changed_paths(data), + ["apps/web/a.ts", "apps/web/b.ts"], + ) + + def test_a_non_ascii_path_survives_and_still_matches(self) -> None: + """The regression this parsing exists for. + + Under git's default `core.quotePath=true`, plain `--name-only` reports + this file as the literal string `"apps/web/caf\\303\\251.ts"`, quotes + and all. That matches no pattern, so a pull request touching only it + would skip every job and report a green gate having tested nothing. + `-z` emits the raw bytes instead. + """ + raw = "apps/web/café.ts\0".encode("utf-8") + parsed = changed_paths.read_changed_paths(raw) + + self.assertEqual(parsed, ["apps/web/café.ts"]) + self.assertTrue(changed_paths.Filter(["apps/web/**"]).matches_any(parsed)) + + quoted = ['"apps/web/caf\\303\\251.ts"'] + self.assertFalse( + changed_paths.Filter(["apps/web/**"]).matches_any(quoted), + "The quoted form must not match -- that is the bug, and this " + "assertion is what proves the -z form above is doing the work.", + ) + + def test_a_path_containing_a_newline_survives_nul_splitting(self) -> None: + data = "apps/web/we ird\nname.ts\0apps/web/b.ts\0".encode("utf-8") + self.assertEqual( + changed_paths.read_changed_paths(data), + ["apps/web/we ird\nname.ts", "apps/web/b.ts"], + ) + + def test_undecodable_bytes_do_not_crash_the_step(self) -> None: + parsed = changed_paths.read_changed_paths(b"apps/web/\xff\xfe.ts\0") + self.assertEqual(len(parsed), 1) + self.assertTrue(changed_paths.Filter(["apps/web/**"]).matches_any(parsed)) + + def test_an_empty_diff_yields_no_paths(self) -> None: + self.assertEqual(changed_paths.read_changed_paths(b""), []) + self.assertEqual(changed_paths.read_changed_paths(b"\0"), []) + + +class CommandLineTest(unittest.TestCase): + def run_script(self, patterns: str, changed: str) -> tuple[int, str, str]: + with tempfile.TemporaryDirectory() as tmp: + output = Path(tmp) / "github_output" + output.touch() + env = { + **os.environ, + "PATTERNS": patterns, + "GITHUB_OUTPUT": str(output), + } + result = subprocess.run( + [sys.executable, str(SCRIPT)], + input=changed, + env=env, + capture_output=True, + text=True, + ) + return result.returncode, result.stdout, output.read_text(encoding="utf-8") + + def test_writes_the_step_output(self) -> None: + code, stdout, step_output = self.run_script( + "apps/web/**\n!**/*.md\n", "apps/web/lib/api.ts\n" + ) + self.assertEqual(code, 0) + self.assertIn("run=true", stdout) + self.assertEqual(step_output, "run=true\n") + + def test_reports_false_without_a_relevant_change(self) -> None: + code, stdout, step_output = self.run_script( + "apps/web/**\n!**/*.md\n", "docs/public/install.md\napps/web/README.md\n" + ) + self.assertEqual(code, 0) + self.assertIn("run=false", stdout) + self.assertEqual(step_output, "run=false\n") + + def test_a_missing_pattern_variable_fails_loudly(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + env = {k: v for k, v in os.environ.items() if k != "PATTERNS"} + env["GITHUB_OUTPUT"] = str(Path(tmp) / "out") + result = subprocess.run( + [sys.executable, str(SCRIPT)], + input="apps/web/lib/api.ts\n", + env=env, + capture_output=True, + text=True, + ) + self.assertNotEqual( + result.returncode, + 0, + "An unset PATTERNS must fail the step. Defaulting to 'run nothing' " + "would pass a merge queue entry that was never tested.", + ) + + +class WorkflowPatternsTest(unittest.TestCase): + """The lists in the workflows must still mean what their filters meant.""" + + def test_each_gating_workflow_selects_and_rejects_the_expected_paths(self) -> None: + for workflow, (selected, rejected) in GATED_WORKFLOWS.items(): + with self.subTest(workflow=workflow): + path_filter = changed_paths.Filter(patterns_block(workflow).splitlines()) + + for path in selected: + self.assertTrue( + path_filter.includes(path), + f"{workflow} must still run for {path}.", + ) + for path in rejected: + self.assertFalse( + path_filter.includes(path), + f"{workflow} must not be selected by {path} alone.", + ) + + def test_every_gating_workflow_filters_on_its_own_definition(self) -> None: + """A change to the workflow file must run the jobs it defines. + + This is the trigger-level `paths:` entry each of these workflows + carried for itself. Losing it means a PR that rewrites the E2E workflow + never runs it. + """ + for workflow in GATED_WORKFLOWS: + with self.subTest(workflow=workflow): + path_filter = changed_paths.Filter(patterns_block(workflow).splitlines()) + self.assertTrue( + path_filter.includes(f".github/workflows/{workflow}"), + f"{workflow} must select itself.", + ) + + def test_gating_workflows_carry_no_trigger_level_path_filter(self) -> None: + """The whole point: these workflows must always report a conclusion. + + A `paths:` key back on `pull_request` or `merge_group` reintroduces the + pending-forever check that blocks a merge queue. + """ + for workflow in GATED_WORKFLOWS: + with self.subTest(workflow=workflow): + text = (WORKFLOWS / workflow).read_text(encoding="utf-8") + triggers = text.partition("\nconcurrency:")[0] + self.assertNotIn( + "\n paths:", + triggers, + f"{workflow} must not filter its triggers by path. The " + "filter belongs in the `changes` job, where a skip still " + "reports a conclusion.", + ) + self.assertIn( + "\n merge_group:\n", + triggers, + f"{workflow} must keep its merge_group trigger; without it " + "the merge queue waits forever for a check that never runs.", + ) + + def test_each_changes_job_feeds_the_script_a_nul_delimited_diff(self) -> None: + """`-z` is load-bearing and the script cannot enforce it from inside. + + Newline-delimited input still parses, by design, so dropping `-z` from + a workflow fails nothing at runtime -- it just quietly restores the + quoting bug for non-ASCII paths, where the gate passes without having + tested the change. This is the only place that can catch it. + """ + for workflow in GATED_WORKFLOWS: + with self.subTest(workflow=workflow): + text = (WORKFLOWS / workflow).read_text(encoding="utf-8") + self.assertIn( + 'git diff --name-only -z "${BASE}" HEAD', + text, + f"{workflow}'s `changes` job must pass -z to git diff. " + "Without it git quotes non-ASCII paths, they match no " + "pattern, and every job is skipped on a green gate.", + ) + + def test_always_run_gates_keep_merge_group_and_stay_unfiltered(self) -> None: + """Same two properties, for the gates that have no `changes` job. + + These are required checks too. Adding a `paths:` filter back, or + dropping the merge_group trigger, stalls the queue on that check just + as surely as it would for the heavy workflows above -- and until now + nothing failed when it happened. + """ + for workflow in ALWAYS_RUN_WORKFLOWS: + with self.subTest(workflow=workflow): + text = (WORKFLOWS / workflow).read_text(encoding="utf-8") + triggers = text.partition("\nconcurrency:")[0] + + self.assertIn( + "\n merge_group:\n", + triggers, + f"{workflow} must keep its merge_group trigger; without it " + "the merge queue waits forever for a check that never runs.", + ) + self.assertNotIn( + "\n paths:", + triggers, + f"{workflow} must not filter its triggers by path. It has " + "no `changes` job to move the filter into, so a filter " + "here means the check simply stops reporting.", + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/.github/scripts/frontend-tests-workflow-contract_test.py b/.github/scripts/frontend-tests-workflow-contract_test.py index 88e5a63755..a846c266ca 100644 --- a/.github/scripts/frontend-tests-workflow-contract_test.py +++ b/.github/scripts/frontend-tests-workflow-contract_test.py @@ -31,10 +31,6 @@ LINT_WORKFLOW = REPO_ROOT / ".github" / "workflows" / "lint-action-pinning.yml" STEP_MARKER = " - name: Run tests\n" NEXT_STEP_MARKER = "\n - name: " -# The subjects above that live outside `.github/`, and so are not covered by -# this job's `.github/workflows/**` trigger. -UNTRIGGERED_SUBJECTS = ("apps/web/vitest.config.ts", "apps/.npmrc") - def run_tests_step(workflow: str) -> str: """Return the body of the `Run tests` step, up to the following step.""" @@ -44,26 +40,12 @@ def run_tests_step(workflow: str) -> str: return remainder.partition(NEXT_STEP_MARKER)[0] -def trigger_paths(workflow: str, trigger: str) -> str: - """Return the `paths:` list of `trigger`'s block under `on:`. - - Scoped to the `paths:` key rather than the whole trigger block. GitHub - rejects `paths` and `paths-ignore` on the same event, so renaming the key - inverts the filter wholesale: the job would then run on everything *except* - these files, and a match against the block as a whole would not notice. - """ +def trigger_block(workflow: str, trigger: str) -> str: + """Return `trigger`'s block under `on:`, up to the next top-level key.""" block = re.search(rf"(?m)^ {trigger}:\n(?:^ {{4}}.*\n|^\n)*", workflow) if block is None: raise AssertionError(f"lint-action-pinning.yml has no {trigger} trigger") - - paths = re.search(r"(?m)^ paths:\n((?:^ - .*\n)+)", block.group(0)) - if paths is None: - raise AssertionError( - f"lint-action-pinning.yml's {trigger} trigger has no `paths:` list. A " - "`paths-ignore:` key in its place inverts the filter, and the job stops " - "running for the very files listed under it." - ) - return paths.group(1) + return block.group(0) class FrontendTestsWorkflowContractTest(unittest.TestCase): @@ -109,35 +91,40 @@ class FrontendTestsWorkflowContractTest(unittest.TestCase): def test_this_contract_runs_when_its_subjects_change(self) -> None: """The assertions above are only worth anything if CI runs them. - `lint-action-pinning.yml` triggers on `.github/workflows/**`, which - covers `frontend-tests.yml` but neither subject in - `UNTRIGGERED_SUBJECTS`. `frontend-tests.yml` does not cover - `apps/.npmrc` either, so without an explicit trigger here a PR that - deletes `production=false` and nothing else runs no job at all and the - assertion above never executes. Mirrors the equivalent test in - `release-workflow-contract_test.py`. + Two of this file's subjects, `apps/web/vitest.config.ts` and + `apps/.npmrc`, live outside `.github/`. `lint-action-pinning.yml` used + to name them in a hand-maintained `paths:` list so that a PR deleting + `production=false` and nothing else still brought this job up; every + new subject meant remembering to extend that list, and every omission + was a silently unguarded file. - The step assertion at the end is weaker than the path ones by + The list is gone. `lint-action-pinning.yml` now runs on every push and + every pull request, which subsumes the old guarantee for every subject + at once -- and is what lets it be a merge-queue required check, since a + path-filtered workflow reports no conclusion when it is skipped and a + required check that never reports blocks the queue forever. + + The step assertion at the end is weaker than the trigger ones by construction: this file runs only from that step, so the PR that deletes it also deletes the only thing that would fail. It holds for local runs and for a step whose command is renamed or moved elsewhere. Keeping the job a required check is what covers outright removal, and that lives in - branch protection, not here. + the repository ruleset, not here. """ lint_workflow = LINT_WORKFLOW.read_text(encoding="utf-8") - for trigger in ("push", "pull_request"): - paths = trigger_paths(lint_workflow, trigger) + for trigger in ("push", "pull_request", "merge_group"): + block = trigger_block(lint_workflow, trigger) - for subject in UNTRIGGERED_SUBJECTS: - self.assertIn( - f' - "{subject}"\n', - paths, - f"{subject} must be a {trigger} path trigger in " - "lint-action-pinning.yml. It is a subject of this contract " - "but lives outside .github/, so nothing else brings this " - "job up when it changes.", - ) + self.assertNotIn( + " paths:", + block, + f"lint-action-pinning.yml's {trigger} trigger must stay " + "unfiltered. A `paths:` list there reintroduces both failure " + "modes it was removed for: subjects outside the list stop " + "being guarded, and a skipped run reports no conclusion, which " + "a merge queue waits on forever.", + ) self.assertIn( "\n run: python3 .github/scripts/frontend-tests-workflow-contract_test.py\n", diff --git a/.github/scripts/release-workflow-contract_test.py b/.github/scripts/release-workflow-contract_test.py index 9af71bf3ed..0e7f13b4c6 100644 --- a/.github/scripts/release-workflow-contract_test.py +++ b/.github/scripts/release-workflow-contract_test.py @@ -442,32 +442,33 @@ class ReleaseWorkflowContractTest(unittest.TestCase): self.assertEqual(primary_fingerprints[0], documented_fingerprint.group(1)) def test_release_contract_ci_runs_when_key_or_release_documentation_changes(self) -> None: - for trigger in ("push", "pull_request"): + """This contract must run on every change, not a listed subset. + + `lint-action-pinning.yml` used to name each subject of this file -- + the signing key, the release docs, every `scripts/release/` helper -- + in a hand-maintained `paths:` list, so that changing one brought the + job up. That list is gone: the workflow now triggers on every push and + every pull request, which covers every subject here and every one added + later without anybody remembering to extend a list. + + The change was forced by the merge queue. A workflow skipped by a + `paths:` filter reports no conclusion at all, and a required check that + never reports blocks a pull request from entering the queue. + """ + for trigger in ("push", "pull_request", "merge_group"): trigger_block = re.search( rf" {trigger}:\n.*?(?=\n [a-z_]+:|\nconcurrency:)", LINT_WORKFLOW, re.DOTALL, ) self.assertIsNotNone(trigger_block) - self.assertIn('".github/release-signing-key.asc"', trigger_block.group(0)) - self.assertIn('"docs/public/release-process.md"', trigger_block.group(0)) - self.assertIn('"scripts/release/package-npm-runtime.sh"', trigger_block.group(0)) - self.assertIn('"scripts/release/publish-npm.sh"', trigger_block.group(0)) - self.assertIn('"scripts/release/publish-npm.test.mjs"', trigger_block.group(0)) - self.assertIn('"scripts/release/npm-packages.sh"', trigger_block.group(0)) - self.assertIn('"scripts/release/npm-view-version.sh"', trigger_block.group(0)) - self.assertIn('"scripts/release/npm-view-version.test.mjs"', trigger_block.group(0)) - self.assertIn('"scripts/release/nightly-version.mjs"', trigger_block.group(0)) - self.assertIn('"scripts/release/nightly-version.test.mjs"', trigger_block.group(0)) - self.assertIn('"scripts/release/nightly-release.sh"', trigger_block.group(0)) - self.assertIn('"scripts/release/nightly-release.test.mjs"', trigger_block.group(0)) - self.assertIn('"Makefile"', trigger_block.group(0)) - self.assertIn('"apps/backend/Makefile"', trigger_block.group(0)) - self.assertIn('"scripts/release/package-bundle.sh"', trigger_block.group(0)) - self.assertIn('"scripts/release/runtime-bundle.test.sh"', trigger_block.group(0)) - self.assertIn( - '"scripts/release/validate-darwin-arm64-helper.mjs"', + self.assertNotIn( + " paths:", trigger_block.group(0), + f"lint-action-pinning.yml's {trigger} trigger must stay " + "unfiltered. A `paths:` list there both un-guards every " + "subject it omits and makes the check unreportable in a merge " + "queue.", ) setup_node = ( diff --git a/.github/workflows/architecture-lint.yml b/.github/workflows/architecture-lint.yml index 615aaf469d..c6eb63a80d 100644 --- a/.github/workflows/architecture-lint.yml +++ b/.github/workflows/architecture-lint.yml @@ -5,11 +5,15 @@ on: branches: [main] pull_request: branches: [main] + merge_group: + types: [checks_requested] concurrency: # Pull requests can supersede one another safely. Pushes compare each commit # with its own before-SHA, so never cancel a push run before that comparison - # completes. + # completes. Merge-queue runs are keyed by their own merge-group SHA for the + # same reason, and are likewise never cancelled: the queue reads a cancelled + # check as a failure and ejects the pull request. group: ${{ github.workflow }}-${{ github.event_name == 'pull_request' && github.ref || github.sha }} cancel-in-progress: ${{ github.event_name == 'pull_request' }} @@ -31,19 +35,42 @@ jobs: - name: Run linter tests run: python3 scripts/lint-architecture.test.py - - name: Lint pull request architecture - if: github.event_name == 'pull_request' + # One step rather than one per event. Each event names the baseline + # commit differently, and a `merge_group` run has neither of the two the + # split steps used to read: no `event.pull_request`, no `event.before`. + # Under the old shape it would have run the linter tests and silently + # linted nothing, which is the worst possible outcome for a required + # check — green, and having compared the branch against nothing. + - name: Resolve the architecture baseline + id: baseline env: - ARCHITECTURE_BASE_REF: ${{ github.event.pull_request.base.sha }} - run: >- - python3 scripts/lint-architecture.py --all - --baseline-base-ref "$ARCHITECTURE_BASE_REF" - --allow-missing-base-baseline + EVENT_NAME: ${{ github.event_name }} + PR_BASE_SHA: ${{ github.event.pull_request.base.sha }} + MERGE_GROUP_BASE_SHA: ${{ github.event.merge_group.base_sha }} + PUSH_BEFORE: ${{ github.event.before }} + run: | + set -euo pipefail - - name: Lint pushed architecture - if: github.event_name == 'push' + case "${EVENT_NAME}" in + pull_request) BASE_REF="${PR_BASE_SHA}" ;; + # The queue branch is cut from this commit, so it is the merge + # group's equivalent of the pull request's base. + merge_group) BASE_REF="${MERGE_GROUP_BASE_SHA}" ;; + push) BASE_REF="${PUSH_BEFORE}" ;; + *) BASE_REF="" ;; + esac + + if [[ -z "${BASE_REF}" ]]; then + echo "No baseline commit for event '${EVENT_NAME}'." >&2 + exit 1 + fi + + echo "base-ref=${BASE_REF}" >> "$GITHUB_OUTPUT" + echo "Architecture baseline: ${BASE_REF}" + + - name: Lint architecture env: - ARCHITECTURE_BASE_REF: ${{ github.event.before }} + ARCHITECTURE_BASE_REF: ${{ steps.baseline.outputs.base-ref }} run: >- python3 scripts/lint-architecture.py --all --baseline-base-ref "$ARCHITECTURE_BASE_REF" diff --git a/.github/workflows/backend-tests.yml b/.github/workflows/backend-tests.yml index 24c46b7d60..88bf00a67d 100644 --- a/.github/workflows/backend-tests.yml +++ b/.github/workflows/backend-tests.yml @@ -1,32 +1,25 @@ name: Backend Tests +# No trigger-level `paths:` filter, deliberately. A workflow skipped by one +# reports no conclusion at all, and a required check that never reports leaves +# a pull request unable to enter a merge queue and a merge group unable to +# leave it. `merge_group` cannot carry a `paths:` filter in the first place. +# The filter lives in the `changes` job below instead, and the `test` job +# reports the single conclusion the queue waits on. See docs/ci-merge-queue.md. on: push: branches: [main] - paths: - - "apps/backend/**" - # check-make-shells also lints the root Makefile's $(shell ...) probes, so - # a change there has to be able to trip it. - - "Makefile" - - "scripts/check-make-shells" - - ".github/workflows/backend-tests.yml" - - "!**/*.md" - - "!**/*.mdx" - - "!**/*.markdown" pull_request: branches: [main] - paths: - - "apps/backend/**" - - "Makefile" - - "scripts/check-make-shells" - - ".github/workflows/backend-tests.yml" - - "!**/*.md" - - "!**/*.mdx" - - "!**/*.markdown" + merge_group: + types: [checks_requested] concurrency: group: ${{ github.workflow }}-${{ github.ref }} - cancel-in-progress: true + # Never cancel a merge-queue run. Its ref is unique per queue entry, so a + # cancellation here could only come from a mis-keyed group, and the queue + # would read the cancelled check as a failure and eject the pull request. + cancel-in-progress: ${{ github.event_name != 'merge_group' }} # Least-privilege default. The Linux static/test jobs and the Windows `test-windows` # job only read source and run tests — no PR/issue writes, no release ops. @@ -39,8 +32,78 @@ permissions: packages: read jobs: + # The former trigger-level `paths:` list, applied one level down so a skip + # still produces a conclusion. `.github/scripts/changed-paths.py` implements + # the same last-match-wins semantics GitHub applies to `paths:`. + changes: + name: Detect relevant changes + runs-on: ubuntu-latest + timeout-minutes: 10 + outputs: + run: ${{ steps.detect.outputs.run }} + + steps: + - name: Checkout code + uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6 + with: + # The base commit has to be present locally for the diff below. Every + # other job in this workflow already pays for a full clone. + fetch-depth: 0 + persist-credentials: false + + - name: Detect relevant changes + id: detect + env: + PATTERNS: | + apps/backend/** + # check-make-shells also lints the root Makefile's $(shell ...) + # probes, so a change there has to be able to trip it. + Makefile + scripts/check-make-shells + .github/workflows/backend-tests.yml + .github/scripts/changed-paths.py + !**/*.md + !**/*.mdx + !**/*.markdown + EVENT_NAME: ${{ github.event_name }} + BASE_REF: ${{ github.base_ref }} + MERGE_GROUP_BASE_SHA: ${{ github.event.merge_group.base_sha }} + PUSH_BEFORE: ${{ github.event.before }} + run: | + set -euo pipefail + + case "${EVENT_NAME}" in + # The checkout is the merge ref, whose first parent is the base + # branch tip. Diffing against that tip directly collapses to a + # two-dot diff over everything main landed since the PR opened, so + # take the fork point instead. + pull_request) BASE="$(git merge-base HEAD "origin/${BASE_REF}")" ;; + # The queue branch is cut from this commit, so it is a true ancestor. + merge_group) BASE="${MERGE_GROUP_BASE_SHA}" ;; + push) BASE="${PUSH_BEFORE}" ;; + *) BASE="" ;; + esac + + # Fail open. A first push to a branch, a force push, or an unknown + # event leaves no usable base, and running the suite needlessly is + # cheaper than merging something no job ever looked at. + if [[ -z "${BASE}" || "${BASE}" == "0000000000000000000000000000000000000000" ]] \ + || ! git cat-file -e "${BASE}^{commit}" 2>/dev/null; then + echo "No usable base commit for '${EVENT_NAME}'; running everything." + echo "run=true" >> "$GITHUB_OUTPUT" + exit 0 + fi + + # `-z`, not plain --name-only: git quotes non-ASCII paths under its + # default core.quotePath, and a quoted path matches no pattern, so a + # change to one would skip every job and still report success. + git diff --name-only -z "${BASE}" HEAD \ + | python3 .github/scripts/changed-paths.py + static_checks: name: Backend Static Checks + needs: changes + if: needs.changes.outputs.run == 'true' runs-on: ubuntu-latest # Pre-baked image bundles Go, golangci-lint, go-licenses, Docker CLI, Node, # pnpm, and the Playwright base — see .github/docker/ci-base/Dockerfile. @@ -94,13 +157,23 @@ jobs: - name: Resolve base SHA shell: bash working-directory: ${{ github.workspace }} + env: + EVENT_NAME: ${{ github.event_name }} + PR_BASE_SHA: ${{ github.event.pull_request.base.sha }} + MERGE_GROUP_BASE_SHA: ${{ github.event.merge_group.base_sha }} + PUSH_BEFORE: ${{ github.event.before }} run: | set -euo pipefail - if [[ "${{ github.event_name }}" == "pull_request" ]]; then - BASE_SHA="${{ github.event.pull_request.base.sha }}" + # A merge_group run has no pull request behind it, so neither + # `pull_request.base.sha` nor `event.before` exists. The queue branch + # is cut from `merge_group.base_sha`, which is the equivalent base. + if [[ "${EVENT_NAME}" == "pull_request" ]]; then + BASE_SHA="${PR_BASE_SHA}" + elif [[ "${EVENT_NAME}" == "merge_group" ]]; then + BASE_SHA="${MERGE_GROUP_BASE_SHA}" else - BASE_SHA="${{ github.event.before }}" + BASE_SHA="${PUSH_BEFORE}" fi if [[ -z "${BASE_SHA}" || "${BASE_SHA}" == "0000000000000000000000000000000000000000" ]]; then @@ -144,6 +217,8 @@ jobs: test_shards: name: Backend Tests (${{ matrix.shard }}/2) + needs: changes + if: needs.changes.outputs.run == 'true' runs-on: ubuntu-latest strategy: fail-fast: false @@ -255,6 +330,8 @@ jobs: test_ambient_env: name: Backend (ambient integration env) + needs: changes + if: needs.changes.outputs.run == 'true' runs-on: ubuntu-latest # test_shards runs in the same clean container and never sets these vars, # so it cannot catch a package that starts reading them again — the class @@ -298,28 +375,78 @@ jobs: GITLAB_TOKEN: glpat-ambient-ci-not-a-real-token run: go test -race ./internal/gitlab/... ./internal/agentctl/server/process/... + # The required check. Runs on every event, including the ones where every job + # above is correctly skipped, so the merge queue always has a conclusion to + # read instead of a check that stays pending forever. + # + # `needs` covers every other job in this workflow on purpose. A merge queue + # gates on check names an admin selects by hand, and a job missing from this + # list is a job whose failure would never block a merge — silently, and + # without appearing anywhere in the ruleset. `postgres-boot` and + # `test-windows` were outside it before this workflow became a gate. test: name: Run Backend Tests runs-on: ubuntu-latest - needs: [static_checks, test_shards, test_ambient_env] + needs: + - changes + - static_checks + - test_shards + - test_ambient_env + - postgres-boot + - test-windows if: ${{ always() }} steps: - name: Require backend checks to pass env: + CHANGES_RESULT: ${{ needs.changes.result }} STATIC_CHECKS_RESULT: ${{ needs.static_checks.result }} TEST_SHARDS_RESULT: ${{ needs.test_shards.result }} TEST_AMBIENT_ENV_RESULT: ${{ needs.test_ambient_env.result }} + POSTGRES_BOOT_RESULT: ${{ needs['postgres-boot'].result }} + TEST_WINDOWS_RESULT: ${{ needs['test-windows'].result }} run: | set -euo pipefail - if [[ "${STATIC_CHECKS_RESULT}" != "success" || "${TEST_SHARDS_RESULT}" != "success" || "${TEST_AMBIENT_ENV_RESULT}" != "success" ]]; then - echo "Static checks: ${STATIC_CHECKS_RESULT}" - echo "Test shards: ${TEST_SHARDS_RESULT}" - echo "Ambient env tests: ${TEST_AMBIENT_ENV_RESULT}" + + echo "Change detection: ${CHANGES_RESULT}" + echo "Static checks: ${STATIC_CHECKS_RESULT}" + echo "Test shards: ${TEST_SHARDS_RESULT}" + echo "Ambient env tests: ${TEST_AMBIENT_ENV_RESULT}" + echo "Postgres boot: ${POSTGRES_BOOT_RESULT}" + echo "Windows tests: ${TEST_WINDOWS_RESULT}" + + # Change detection itself must succeed: if it errored we do not know + # whether the skips below were deliberate. + if [[ "${CHANGES_RESULT}" != "success" ]]; then + echo "Change detection did not succeed; refusing to report a pass." >&2 exit 1 fi + # `skipped` is a pass — the change-detection job decided nothing this + # workflow covers was touched. `cancelled` is not: a cancelled job + # never reached a verdict, and treating that as a pass would let an + # untested merge group through. + # + # This gate is the one check a human opens when a merge is blocked, so + # name the job that failed here rather than making them correlate the + # list above by eye. + for entry in \ + "static_checks:${STATIC_CHECKS_RESULT}" \ + "test_shards:${TEST_SHARDS_RESULT}" \ + "test_ambient_env:${TEST_AMBIENT_ENV_RESULT}" \ + "postgres-boot:${POSTGRES_BOOT_RESULT}" \ + "test-windows:${TEST_WINDOWS_RESULT}"; do + name="${entry%%:*}" + result="${entry#*:}" + if [[ "${result}" != "success" && "${result}" != "skipped" ]]; then + echo "${name} finished with result: ${result}" >&2 + exit 1 + fi + done + postgres-boot: name: Backend Postgres + needs: changes + if: needs.changes.outputs.run == 'true' runs-on: ubuntu-latest permissions: contents: read @@ -386,6 +513,8 @@ jobs: test-windows: name: Backend (windows) + needs: changes + if: needs.changes.outputs.run == 'true' runs-on: windows-latest # The race-sensitive Windows package suite can exceed 15 minutes on a # busy hosted runner; keep the cap finite while allowing normal completion. diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index 121dd34e30..d1eadaae91 100644 --- a/.github/workflows/e2e-tests.yml +++ b/.github/workflows/e2e-tests.yml @@ -1,28 +1,18 @@ name: E2E Tests +# No trigger-level `paths:` filter, deliberately. A workflow skipped by one +# reports no conclusion at all, and a required check that never reports leaves +# a pull request unable to enter a merge queue and a merge group unable to +# leave it. `merge_group` cannot carry a `paths:` filter in the first place. +# The filter lives in the `changes` job below instead, and `e2e-gate` reports +# the single conclusion the queue waits on. See docs/ci-merge-queue.md. on: push: branches: [main] - paths: - - "apps/backend/**" - - "apps/desktop/**" - - "apps/web/**" - - "apps/packages/**" - - "apps/pnpm-lock.yaml" - - "apps/pnpm-workspace.yaml" - - ".github/workflows/e2e-tests.yml" - - "!**/*.md" pull_request: branches: [main] - paths: - - "apps/backend/**" - - "apps/desktop/**" - - "apps/web/**" - - "apps/packages/**" - - "apps/pnpm-lock.yaml" - - "apps/pnpm-workspace.yaml" - - ".github/workflows/e2e-tests.yml" - - "!**/*.md" + merge_group: + types: [checks_requested] # Manual trigger for verifying CI on draft PRs and ad-hoc reruns. The # default sharded run uses the same ref the user picks in the dispatch UI. workflow_dispatch: @@ -35,7 +25,10 @@ on: concurrency: group: ${{ github.workflow }}-${{ github.ref }} - cancel-in-progress: true + # Never cancel a merge-queue run. Its ref is unique per queue entry, so a + # cancellation here could only come from a mis-keyed group, and the queue + # would read the cancelled check as a failure and eject the pull request. + cancel-in-progress: ${{ github.event_name != 'merge_group' }} permissions: contents: read @@ -46,8 +39,77 @@ permissions: packages: read jobs: + # The former trigger-level `paths:` list, applied one level down so a skip + # still produces a conclusion. `.github/scripts/changed-paths.py` implements + # the same last-match-wins semantics GitHub applies to `paths:`. + changes: + name: Detect relevant changes + runs-on: ubuntu-latest + timeout-minutes: 10 + outputs: + run: ${{ steps.detect.outputs.run }} + + steps: + - name: Checkout code + uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6 + with: + # The base commit has to be present locally for the diff below. + fetch-depth: 0 + persist-credentials: false + + - name: Detect relevant changes + id: detect + env: + PATTERNS: | + apps/backend/** + apps/desktop/** + apps/web/** + apps/packages/** + apps/pnpm-lock.yaml + apps/pnpm-workspace.yaml + .github/workflows/e2e-tests.yml + .github/scripts/changed-paths.py + !**/*.md + EVENT_NAME: ${{ github.event_name }} + BASE_REF: ${{ github.base_ref }} + MERGE_GROUP_BASE_SHA: ${{ github.event.merge_group.base_sha }} + PUSH_BEFORE: ${{ github.event.before }} + run: | + set -euo pipefail + + case "${EVENT_NAME}" in + # The checkout is the merge ref, whose first parent is the base + # branch tip. Diffing against that tip directly collapses to a + # two-dot diff over everything main landed since the PR opened, so + # take the fork point instead. + pull_request) BASE="$(git merge-base HEAD "origin/${BASE_REF}")" ;; + # The queue branch is cut from this commit, so it is a true ancestor. + merge_group) BASE="${MERGE_GROUP_BASE_SHA}" ;; + push) BASE="${PUSH_BEFORE}" ;; + # workflow_dispatch is an explicit request to run the suite. + *) BASE="" ;; + esac + + # Fail open. A first push to a branch, a force push, or a manual + # dispatch leaves no usable base, and running the suite needlessly is + # cheaper than merging something no job ever looked at. + if [[ -z "${BASE}" || "${BASE}" == "0000000000000000000000000000000000000000" ]] \ + || ! git cat-file -e "${BASE}^{commit}" 2>/dev/null; then + echo "No usable base commit for '${EVENT_NAME}'; running everything." + echo "run=true" >> "$GITHUB_OUTPUT" + exit 0 + fi + + # `-z`, not plain --name-only: git quotes non-ASCII paths under its + # default core.quotePath, and a quoted path matches no pattern, so a + # change to one would skip every job and still report success. + git diff --name-only -z "${BASE}" HEAD \ + | python3 .github/scripts/changed-paths.py + build: name: Build + needs: changes + if: needs.changes.outputs.run == 'true' runs-on: ubuntu-latest # Pre-baked image bundles Go, Node 24, pnpm 9.15.9 — see # .github/docker/ci-base/Dockerfile. Uses the `build` tag (with Go) @@ -234,7 +296,8 @@ jobs: e2e: name: E2E Shard ${{ matrix.shard }}/14 - needs: build + needs: [changes, build] + if: needs.changes.outputs.run == 'true' runs-on: ubuntu-latest # Pre-baked image bundles Node 24, pnpm 9.15.9, and the Playwright browser # binaries (Chromium etc.) — see .github/docker/ci-base/Dockerfile. This is @@ -361,7 +424,8 @@ jobs: # Sharded 6 ways — each shard is its own runner. See apps/web/e2e/README.md. e2e-containers: name: E2E Containers Shard ${{ matrix.shard }}/6 - needs: build + needs: [changes, build] + if: needs.changes.outputs.run == 'true' # This job intentionally runs on the host ubuntu runner (NOT the kandev-ci # container image): the containers project needs direct access to the # host Docker daemon, and a docker-in-docker setup would break the tests' @@ -410,6 +474,10 @@ jobs: # Their read-only GITHUB_TOKEN can still be rejected by GHCR during an # explicit login because it belongs to the base repository, not the # contributor's fork. Keep authenticated pulls for trusted runs. + # + # A merge_group run has no `github.event.pull_request` at all, so the + # first operand decides it. That is the right answer: the queue branch + # lives in this repository and its token is a base-repository token. - name: Log in to GHCR if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository uses: docker/login-action@dbcb813823bdd20940b903addbd779551569679f # v4.6.0 @@ -531,6 +599,8 @@ jobs: desktop-e2e: name: Desktop E2E Smoke + needs: changes + if: needs.changes.outputs.run == 'true' runs-on: ubuntu-latest # Includes Ubuntu WebKit/Tauri system package installation; GitHub apt mirrors # can spend more than 20 minutes there before tests start. @@ -589,8 +659,12 @@ jobs: e2e-report: name: Merge E2E Reports - needs: [e2e, e2e-containers, desktop-e2e] - if: always() + needs: [changes, e2e, e2e-containers, desktop-e2e] + # `always()` so a failed shard still gets its report merged, but not when + # change detection skipped the shards outright: there would be no blob + # reports to download, and the artifact step would fail on an empty run. + # `e2e-gate` below, not this job, is what the merge queue waits on. + if: always() && needs.changes.outputs.run == 'true' runs-on: ubuntu-latest # Pre-baked image bundles Node 24, pnpm 9.15.9, and the Playwright CLI. # `runtime` tag — no Go needed for merging blob reports. @@ -949,3 +1023,65 @@ jobs: echo "desktop-e2e finished with result: $DESKTOP_E2E_RESULT" >&2 exit 1 fi + + # The required check. Runs on every event, including the ones where every job + # above is correctly skipped, so the merge queue always has a conclusion to + # read instead of a check that stays pending forever. + # + # A single gate is what makes this workflow usable from a ruleset at all: the + # alternative is naming all 14 E2E shards, all 6 container shards, and the + # desktop smoke individually, and renumbering the ruleset every time a shard + # count changes. + e2e-gate: + name: E2E Tests Passed + runs-on: ubuntu-latest + needs: [changes, build, e2e, e2e-containers, desktop-e2e, e2e-report] + if: always() + + steps: + - name: Require the E2E jobs to have passed or been skipped + env: + CHANGES_RESULT: ${{ needs.changes.result }} + BUILD_RESULT: ${{ needs.build.result }} + E2E_RESULT: ${{ needs.e2e.result }} + E2E_CONTAINERS_RESULT: ${{ needs['e2e-containers'].result }} + DESKTOP_E2E_RESULT: ${{ needs['desktop-e2e'].result }} + E2E_REPORT_RESULT: ${{ needs['e2e-report'].result }} + run: | + set -euo pipefail + + echo "Change detection: ${CHANGES_RESULT}" + echo "Build: ${BUILD_RESULT}" + echo "E2E shards: ${E2E_RESULT}" + echo "E2E container shards: ${E2E_CONTAINERS_RESULT}" + echo "Desktop smoke: ${DESKTOP_E2E_RESULT}" + echo "Report merge: ${E2E_REPORT_RESULT}" + + # Change detection itself must succeed: if it errored we do not know + # whether the skips below were deliberate. + if [[ "${CHANGES_RESULT}" != "success" ]]; then + echo "Change detection did not succeed; refusing to report a pass." >&2 + exit 1 + fi + + # `skipped` is a pass — the change-detection job decided nothing this + # workflow covers was touched. `cancelled` is not: a cancelled job + # never reached a verdict, and treating that as a pass would let an + # untested merge group through. + # + # This gate is the one check a human opens when a merge is blocked, so + # name the job that failed here rather than making them correlate the + # list above against twenty shard names. + for entry in \ + "build:${BUILD_RESULT}" \ + "e2e:${E2E_RESULT}" \ + "e2e-containers:${E2E_CONTAINERS_RESULT}" \ + "desktop-e2e:${DESKTOP_E2E_RESULT}" \ + "e2e-report:${E2E_REPORT_RESULT}"; do + name="${entry%%:*}" + result="${entry#*:}" + if [[ "${result}" != "success" && "${result}" != "skipped" ]]; then + echo "${name} finished with result: ${result}" >&2 + exit 1 + fi + done diff --git a/.github/workflows/frontend-tests.yml b/.github/workflows/frontend-tests.yml index 828daac897..bb3f5c7a95 100644 --- a/.github/workflows/frontend-tests.yml +++ b/.github/workflows/frontend-tests.yml @@ -1,42 +1,25 @@ name: Frontend Tests +# No trigger-level `paths:` filter, deliberately. A workflow skipped by one +# reports no conclusion at all, and a required check that never reports leaves +# a pull request unable to enter a merge queue and a merge group unable to +# leave it. `merge_group` cannot carry a `paths:` filter in the first place. +# The filter lives in the `changes` job below instead, and `frontend-gate` +# reports the single conclusion the queue waits on. See docs/ci-merge-queue.md. on: push: branches: [main] - paths: - - "apps/cli/**" - - "apps/package.json" - - "apps/prettier.config.mjs" - - "apps/.prettierignore" - - "apps/web/**" - - "apps/packages/**" - - "apps/pnpm-lock.yaml" - - "apps/backend/go.mod" - - "apps/backend/go.sum" - - ".github/workflows/frontend-tests.yml" - - "!**/*.md" - - "!**/*.mdx" - - "!**/*.markdown" pull_request: branches: [main] - paths: - - "apps/cli/**" - - "apps/package.json" - - "apps/prettier.config.mjs" - - "apps/.prettierignore" - - "apps/web/**" - - "apps/packages/**" - - "apps/pnpm-lock.yaml" - - "apps/backend/go.mod" - - "apps/backend/go.sum" - - ".github/workflows/frontend-tests.yml" - - "!**/*.md" - - "!**/*.mdx" - - "!**/*.markdown" + merge_group: + types: [checks_requested] concurrency: group: ${{ github.workflow }}-${{ github.ref }} - cancel-in-progress: true + # Never cancel a merge-queue run. Its ref is unique per queue entry, so a + # cancellation here could only come from a mis-keyed group, and the queue + # would read the cancelled check as a failure and eject the pull request. + cancel-in-progress: ${{ github.event_name != 'merge_group' }} # Least-privilege: the `frontend` job only reads source and runs tests. # `packages: read` lets the container job pull the kandev-ci image from GHCR. @@ -45,8 +28,82 @@ permissions: packages: read jobs: + # The former trigger-level `paths:` list, applied one level down so a skip + # still produces a conclusion. `.github/scripts/changed-paths.py` implements + # the same last-match-wins semantics GitHub applies to `paths:`. + changes: + name: Detect relevant changes + runs-on: ubuntu-latest + timeout-minutes: 10 + outputs: + run: ${{ steps.detect.outputs.run }} + + steps: + - name: Checkout code + uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6 + with: + # The base commit has to be present locally for the diff below. Every + # other job in this workflow already pays for a full clone. + fetch-depth: 0 + persist-credentials: false + + - name: Detect relevant changes + id: detect + env: + PATTERNS: | + apps/cli/** + apps/package.json + apps/prettier.config.mjs + apps/.prettierignore + apps/web/** + apps/packages/** + apps/pnpm-lock.yaml + apps/backend/go.mod + apps/backend/go.sum + .github/workflows/frontend-tests.yml + .github/scripts/changed-paths.py + !**/*.md + !**/*.mdx + !**/*.markdown + EVENT_NAME: ${{ github.event_name }} + BASE_REF: ${{ github.base_ref }} + MERGE_GROUP_BASE_SHA: ${{ github.event.merge_group.base_sha }} + PUSH_BEFORE: ${{ github.event.before }} + run: | + set -euo pipefail + + case "${EVENT_NAME}" in + # The checkout is the merge ref, whose first parent is the base + # branch tip. Diffing against that tip directly collapses to a + # two-dot diff over everything main landed since the PR opened, so + # take the fork point -- the same correction the steps below make. + pull_request) BASE="$(git merge-base HEAD "origin/${BASE_REF}")" ;; + # The queue branch is cut from this commit, so it is a true ancestor. + merge_group) BASE="${MERGE_GROUP_BASE_SHA}" ;; + push) BASE="${PUSH_BEFORE}" ;; + *) BASE="" ;; + esac + + # Fail open. A first push to a branch, a force push, or an unknown + # event leaves no usable base, and running the suite needlessly is + # cheaper than merging something no job ever looked at. + if [[ -z "${BASE}" || "${BASE}" == "0000000000000000000000000000000000000000" ]] \ + || ! git cat-file -e "${BASE}^{commit}" 2>/dev/null; then + echo "No usable base commit for '${EVENT_NAME}'; running everything." + echo "run=true" >> "$GITHUB_OUTPUT" + exit 0 + fi + + # `-z`, not plain --name-only: git quotes non-ASCII paths under its + # default core.quotePath, and a quoted path matches no pattern, so a + # change to one would skip every job and still report success. + git diff --name-only -z "${BASE}" HEAD \ + | python3 .github/scripts/changed-paths.py + frontend: name: Run Frontend Lint, Tests, and Build + needs: changes + if: needs.changes.outputs.run == 'true' runs-on: ubuntu-latest # Pre-baked image bundles Node 24, pnpm 9.15.9, Go, and go-licenses — # see .github/docker/ci-base/Dockerfile. @@ -101,6 +158,11 @@ jobs: - name: Check formatting shell: bash + env: + EVENT_NAME: ${{ github.event_name }} + BASE_REF: ${{ github.base_ref }} + MERGE_GROUP_BASE_SHA: ${{ github.event.merge_group.base_sha }} + PUSH_BEFORE: ${{ github.event.before }} run: | set -euo pipefail @@ -112,10 +174,16 @@ jobs: # extra files are already formatted), but it is the same trap the i18n # ratchet below hit for real, so do not copy the old idiom. The fork # point is what "changed in this PR" actually means. - if [[ "${{ github.event_name }}" == "pull_request" ]]; then - BASE_SHA="$(git merge-base HEAD "origin/${{ github.base_ref }}")" + # + # `merge_group` has no PR context at all. Its own base_sha is the + # commit the queue branch was cut from, which is already the fork + # point, so no correction is needed there. + if [[ "${EVENT_NAME}" == "pull_request" ]]; then + BASE_SHA="$(git merge-base HEAD "origin/${BASE_REF}")" + elif [[ "${EVENT_NAME}" == "merge_group" ]]; then + BASE_SHA="${MERGE_GROUP_BASE_SHA}" else - BASE_SHA="${{ github.event.before }}" + BASE_SHA="${PUSH_BEFORE}" fi if [[ -z "${BASE_SHA}" || "${BASE_SHA}" == "0000000000000000000000000000000000000000" ]]; then @@ -183,6 +251,10 @@ jobs: shell: bash # The job defaults to `apps`; these scripts live one level deeper. working-directory: apps/web + env: + EVENT_NAME: ${{ github.event_name }} + MERGE_GROUP_BASE_SHA: ${{ github.event.merge_group.base_sha }} + PUSH_BEFORE: ${{ github.event.before }} run: | set -euo pipefail @@ -194,14 +266,23 @@ jobs: # (#2160). The scripts' own default, `merge-base HEAD origin/main`, # resolves to the merge ref's base-side parent and is correct here; # origin/main is present because the checkout is fetch-depth: 0. - if [[ "${{ github.event_name }}" == "pull_request" ]]; then + if [[ "${EVENT_NAME}" == "pull_request" ]]; then node scripts/run-i18n-ratchet.mjs exit 0 fi - # push: HEAD *is* the new base branch tip, so there is no fork point to - # find and the previous tip is the only meaningful base. - BASE_SHA="${{ github.event.before }}" + # merge_group: the queue branch is cut from base_sha and carries only + # the queued pull requests on top, so that IS the fork point. The + # scripts' `origin/main` default is wrong here — main may already have + # moved past what this merge group was built on. + if [[ "${EVENT_NAME}" == "merge_group" ]]; then + BASE_SHA="${MERGE_GROUP_BASE_SHA}" + else + # push: HEAD *is* the new base branch tip, so there is no fork point to + # find and the previous tip is the only meaningful base. + BASE_SHA="${PUSH_BEFORE}" + fi + if [[ -z "${BASE_SHA}" || "${BASE_SHA}" == "0000000000000000000000000000000000000000" ]]; then echo "No usable base SHA; skipping the new-code ratchet." exit 0 @@ -223,15 +304,24 @@ jobs: - name: Run E2E sleep ratchet shell: bash working-directory: apps/web + env: + EVENT_NAME: ${{ github.event_name }} + MERGE_GROUP_BASE_SHA: ${{ github.event.merge_group.base_sha }} + PUSH_BEFORE: ${{ github.event.before }} run: | set -euo pipefail - if [[ "${{ github.event_name }}" == "pull_request" ]]; then + if [[ "${EVENT_NAME}" == "pull_request" ]]; then node scripts/check-new-e2e-sleeps.mjs exit 0 fi - BASE_SHA="${{ github.event.before }}" + if [[ "${EVENT_NAME}" == "merge_group" ]]; then + BASE_SHA="${MERGE_GROUP_BASE_SHA}" + else + BASE_SHA="${PUSH_BEFORE}" + fi + if [[ -z "${BASE_SHA}" || "${BASE_SHA}" == "0000000000000000000000000000000000000000" ]]; then echo "No usable base SHA; skipping the E2E sleep ratchet." exit 0 @@ -256,3 +346,34 @@ jobs: - name: Build run: pnpm --filter @kandev/web build + + # The required check. Runs on every event, including the ones where the + # `frontend` job above is correctly skipped, so the merge queue always has a + # conclusion to read instead of a check that stays pending forever. + frontend-gate: + name: Frontend Tests Passed + runs-on: ubuntu-latest + needs: [changes, frontend] + if: always() + + steps: + - name: Require the frontend job to have passed or been skipped + env: + CHANGES_RESULT: ${{ needs.changes.result }} + FRONTEND_RESULT: ${{ needs.frontend.result }} + run: | + set -euo pipefail + + # `skipped` is a pass: the change-detection job decided nothing this + # workflow covers was touched. `cancelled` is not — a cancelled job + # never reached a verdict, and treating that as a pass would let an + # untested merge group through. + if [[ "${CHANGES_RESULT}" != "success" ]]; then + echo "Change detection finished with result: ${CHANGES_RESULT}" >&2 + exit 1 + fi + if [[ "${FRONTEND_RESULT}" != "success" && "${FRONTEND_RESULT}" != "skipped" ]]; then + echo "Frontend job finished with result: ${FRONTEND_RESULT}" >&2 + exit 1 + fi + echo "Change detection: ${CHANGES_RESULT}; frontend: ${FRONTEND_RESULT}" diff --git a/.github/workflows/lint-action-pinning.yml b/.github/workflows/lint-action-pinning.yml index 5f937bd575..bbfba90814 100644 --- a/.github/workflows/lint-action-pinning.yml +++ b/.github/workflows/lint-action-pinning.yml @@ -1,74 +1,30 @@ name: Lint Action Pinning +# No trigger-level `paths:` filter, deliberately. A workflow skipped by one +# reports no conclusion at all, and a required check that never reports leaves +# a pull request unable to enter a merge queue and a merge group unable to +# leave it. `merge_group` cannot carry a `paths:` filter in the first place. +# +# The heavier gates push their filtering down into a `changes` job (see +# `.github/scripts/changed-paths.py`). This one is deliberately unfiltered +# instead: the list it replaced had grown to thirty-odd entries that each had +# to be added by hand whenever a contract test gained a new subject, and every +# omission was a silently unguarded file. Always running is both cheaper to +# reason about and strictly safer. See docs/ci-merge-queue.md. on: push: branches: [main] - paths: - - ".github/workflows/**" - - ".github/scripts/lint-action-pinning.py" - - ".github/scripts/lint-action-pinning_test.py" - - ".github/scripts/claude-code-review-workflow-contract_test.py" - - ".github/scripts/frontend-tests-workflow-contract_test.py" - - "apps/web/vitest.config.ts" - - "apps/.npmrc" - - ".github/scripts/preview-env-workflow-contract_test.py" - - ".github/scripts/collect-macos-desktop-diagnostics.sh" - - ".github/scripts/release-workflow-contract_test.py" - - ".github/release-signing-key.asc" - - "docs/public/release-process.md" - - "Makefile" - - "apps/backend/Makefile" - - "scripts/release/package-bundle.sh" - - "scripts/release/runtime-bundle.test.sh" - - "scripts/release/validate-darwin-arm64-helper.mjs" - - "scripts/release/npm-packages.sh" - - "scripts/release/nightly-version.mjs" - - "scripts/release/nightly-version.test.mjs" - - "scripts/release/nightly-release.sh" - - "scripts/release/nightly-release.test.mjs" - - "scripts/release/npm-view-version.sh" - - "scripts/release/npm-view-version.test.mjs" - - "scripts/release/package-npm-runtime.sh" - - "scripts/release/publish-npm.sh" - - "scripts/release/publish-npm.test.mjs" - - "scripts/release/retry-ghcr-command.sh" - - "scripts/release/retry-ghcr-command.test.sh" pull_request: branches: [main] - paths: - - ".github/workflows/**" - - ".github/scripts/lint-action-pinning.py" - - ".github/scripts/lint-action-pinning_test.py" - - ".github/scripts/claude-code-review-workflow-contract_test.py" - - ".github/scripts/frontend-tests-workflow-contract_test.py" - - "apps/web/vitest.config.ts" - - "apps/.npmrc" - - ".github/scripts/preview-env-workflow-contract_test.py" - - ".github/scripts/collect-macos-desktop-diagnostics.sh" - - ".github/scripts/release-workflow-contract_test.py" - - ".github/release-signing-key.asc" - - "docs/public/release-process.md" - - "Makefile" - - "apps/backend/Makefile" - - "scripts/release/package-bundle.sh" - - "scripts/release/runtime-bundle.test.sh" - - "scripts/release/validate-darwin-arm64-helper.mjs" - - "scripts/release/npm-packages.sh" - - "scripts/release/nightly-version.mjs" - - "scripts/release/nightly-version.test.mjs" - - "scripts/release/nightly-release.sh" - - "scripts/release/nightly-release.test.mjs" - - "scripts/release/npm-view-version.sh" - - "scripts/release/npm-view-version.test.mjs" - - "scripts/release/package-npm-runtime.sh" - - "scripts/release/publish-npm.sh" - - "scripts/release/publish-npm.test.mjs" - - "scripts/release/retry-ghcr-command.sh" - - "scripts/release/retry-ghcr-command.test.sh" + merge_group: + types: [checks_requested] concurrency: group: ${{ github.workflow }}-${{ github.ref }} - cancel-in-progress: true + # Never cancel a merge-queue run. Its ref is unique per queue entry, so a + # cancellation here could only come from a mis-keyed group, and the queue + # would read the cancelled check as a failure and eject the pull request. + cancel-in-progress: ${{ github.event_name != 'merge_group' }} # Least-privilege: only reads source. permissions: @@ -88,6 +44,9 @@ jobs: - name: Test action-pinning lint run: python3 .github/scripts/lint-action-pinning_test.py + - name: Test in-job path filter + run: python3 .github/scripts/changed-paths_test.py + - name: Test Claude Code review workflow contract run: python3 .github/scripts/claude-code-review-workflow-contract_test.py diff --git a/.github/workflows/lint-harness-files.yml b/.github/workflows/lint-harness-files.yml index 6d1dfdb753..26bbadc3e8 100644 --- a/.github/workflows/lint-harness-files.yml +++ b/.github/workflows/lint-harness-files.yml @@ -1,70 +1,30 @@ name: Lint Harness Files +# No trigger-level `paths:` filter, deliberately. A workflow skipped by one +# reports no conclusion at all, and a required check that never reports leaves +# a pull request unable to enter a merge queue and a merge group unable to +# leave it. `merge_group` cannot carry a `paths:` filter in the first place. +# +# The heavier gates push their filtering down into a `changes` job (see +# `.github/scripts/changed-paths.py`). This job is a checkout plus four short +# linters, so the whole list is replaced by simply always running: cheaper than +# the job that would decide whether to run it, and it closes the gap where a +# harness file outside the old list changed and nothing checked it. +# See docs/ci-merge-queue.md. on: push: branches: [main] - paths: - - ".github/workflows/lint-harness-files.yml" - - ".github/scripts/lint-harness-files.py" - - "scripts/lint-harness-files.test.py" - - ".pre-commit-config.yaml" - - "AGENTS.md" - - "CLAUDE.md" - - "**/AGENTS.md" - - "**/CLAUDE.md" - - "**/.cursorrules" - - ".agents/skills/**/*.md" - - ".augment/skills/**/*.md" - - ".claude/skills/**/*.md" - - ".cursor/skills/**/*.md" - - ".opencode/skills/**/*.md" - - "apps/backend/internal/office/configloader/skills/**/*.md" - - ".agents/agents/**/*.md" - - ".opencode/agents/**/*.md" - - ".claude/agents/**/*.md" - - ".codex/agents/**/*.toml" - - ".codex/config.toml" - - ".claude/settings.json" - - ".claude/rules/**/*.md" - - ".cursor/rules/**/*.mdc" - - ".claude/commands/**/*.md" - - "apps/web/lib/plugins/types.ts" - - "scripts/validate-plugin-api-claims.mjs" - - "scripts/validate-plugin-api-claims.test.mjs" pull_request: branches: [main] - paths: - - ".github/workflows/lint-harness-files.yml" - - ".github/scripts/lint-harness-files.py" - - "scripts/lint-harness-files.test.py" - - ".pre-commit-config.yaml" - - "AGENTS.md" - - "CLAUDE.md" - - "**/AGENTS.md" - - "**/CLAUDE.md" - - "**/.cursorrules" - - ".agents/skills/**/*.md" - - ".augment/skills/**/*.md" - - ".claude/skills/**/*.md" - - ".cursor/skills/**/*.md" - - ".opencode/skills/**/*.md" - - "apps/backend/internal/office/configloader/skills/**/*.md" - - ".agents/agents/**/*.md" - - ".opencode/agents/**/*.md" - - ".claude/agents/**/*.md" - - ".codex/agents/**/*.toml" - - ".codex/config.toml" - - ".claude/settings.json" - - ".claude/rules/**/*.md" - - ".cursor/rules/**/*.mdc" - - ".claude/commands/**/*.md" - - "apps/web/lib/plugins/types.ts" - - "scripts/validate-plugin-api-claims.mjs" - - "scripts/validate-plugin-api-claims.test.mjs" + merge_group: + types: [checks_requested] concurrency: group: ${{ github.workflow }}-${{ github.ref }} - cancel-in-progress: true + # Never cancel a merge-queue run. Its ref is unique per queue entry, so a + # cancellation here could only come from a mis-keyed group, and the queue + # would read the cancelled check as a failure and eject the pull request. + cancel-in-progress: ${{ github.event_name != 'merge_group' }} permissions: contents: read diff --git a/apps/web/scripts/lib/e2e-sleep-wiring.test.ts b/apps/web/scripts/lib/e2e-sleep-wiring.test.ts index a5df5fb993..69712ad43a 100644 --- a/apps/web/scripts/lib/e2e-sleep-wiring.test.ts +++ b/apps/web/scripts/lib/e2e-sleep-wiring.test.ts @@ -58,9 +58,16 @@ describe("sleep ratchet wiring", () => { // would move the slice boundary and quietly shrink what is asserted // (review finding). This is strictly tighter than the slice was — it pins // the invocation, its bare form, and the early exit, in order. + // + // How the event name is read is deliberately NOT pinned. It was + // `"${{ github.event_name }}"` inline until the merge-queue work moved it + // to an `EVENT_NAME` env var, which changed nothing about the guarantee + // here and broke this assertion anyway. What matters is that the + // pull_request branch runs the ratchet bare and returns: the trailing + // newline after the script name is what rejects a smuggled `--base`. expect(step).toMatch( new RegExp( - `if \\[\\[ "\\$\\{\\{ github.event_name \\}\\}" == "pull_request" \\]\\];? then\\n` + + `== "pull_request" \\]\\];? then\\n` + `\\s+node scripts/${RATCHET_SCRIPT}\\n` + `\\s+exit 0\\n`, ), diff --git a/docs/ci-merge-queue.md b/docs/ci-merge-queue.md new file mode 100644 index 0000000000..5f5787da3f --- /dev/null +++ b/docs/ci-merge-queue.md @@ -0,0 +1,204 @@ +# Enabling the merge queue + +Everything in this document except the final section is already in the +repository. Nothing here is switched on: the workflows are prepared, and the +ruleset change that turns a merge queue on is a separate, deliberate decision. + +## Why + +`main`'s ruleset (id `13341245`) has `deletion`, `non_fast_forward`, +`required_linear_history`, and `pull_request` rules, and no +`required_status_checks` rule at all. Merges are not gated on CI in any way. + +That is how #2681 (`38c94e7d0`) landed red. Its E2E checks were fully green, but +they had run 23 hours earlier against a merge ref that predated the retry loop +added by #2569 (`15788438b`). Neither change is wrong on its own; together they +put a roughly 4.5-second floor under content search, just over the 5-second +default assertion in the test #2681 added. Both pull requests were green, and +their merge was not. + +Required status checks alone would not have caught it -- #2681's checks were +green. A merge queue would: it builds the actual merge result and runs the +checks against that, at merge time, in queue order. + +## What changed in the workflows + +### The `merge_group` trigger + +Six workflows gained `merge_group: types: [checks_requested]`: + +| Workflow | Gate job | Check name to require | +| ------------------------ | -------------- | --------------------------------------- | +| `e2e-tests.yml` | `e2e-gate` | `E2E Tests Passed` | +| `backend-tests.yml` | `test` | `Run Backend Tests` | +| `frontend-tests.yml` | `frontend-gate`| `Frontend Tests Passed` | +| `architecture-lint.yml` | `lint` | `Architecture boundaries do not regress`| +| `lint-action-pinning.yml`| `lint` | `All action refs are SHA-pinned` | +| `lint-harness-files.yml` | `lint` | `Harness files pass lint` | + +Deliberately excluded, because they cannot report in a queue and would stall it +forever, or because their redness is not a property of the merge: + +- `pr-title.yml`, `claude-code-review.yml`, `opencode-code-review.yml`, + `preview-env.yml`, `notify-docs.yml` -- all `pull_request` or + `pull_request_target` scoped. There is no pull request in a merge group, so + they never run and never report. +- `cargo-audit.yml` -- also runs on a weekly `schedule`, by design: a newly + published RUSTSEC advisory turns it red with no change to the code. In a queue + that would block every merge in the repository until somebody bumped a crate. + It stays a pull-request and scheduled check. +- `ci-base-image.yml`, `universal-rebuild.yml` -- these publish images rather + than test a merge. +- `release.yml` and the plugin registry workflows -- out of scope. + +### The path-filter problem, and the approach chosen + +Most of these workflows were heavily path-filtered (`apps/web/**`, `!**/*.md`, +and so on). That is a problem for a queue in two independent ways. + +1. A workflow skipped by a trigger-level `paths:` filter reports **no + conclusion at all**. GitHub's documentation is explicit that "checks + associated with that workflow will remain in a 'Pending' state". A required + check that stays pending blocks a pull request from being added to the queue, + and blocks a merge group from ever merging. +2. `merge_group` does not support `paths:` in the first place. It takes `types:` + and `branches:` only. So the filters could not simply be mirrored onto the + queue side even if we wanted them there. + +Two approaches were available: mirror each filter onto an inverse trigger with a +"skipped reports success" shim job, or make the gating jobs always run and +short-circuit internally. + +**The shim was rejected, and the internal short-circuit implemented.** The +deciding factor is that `paths` with `!` exclusions has no exact inverse. Take +frontend-tests' old filter, `apps/web/**` followed by `!**/*.md`. A pull request +that touches only `apps/web/README.md` is skipped by that filter, and is *also* +skipped by the mirrored `paths-ignore` list, because every changed file appears +in it. Both halves skip, no conclusion is ever reported, and the queue waits +forever -- the exact failure the shim exists to prevent, reintroduced by the +shim. Making the inversion correct would mean hand-deriving a second pattern +list per workflow and keeping the two in sync by eye. Worse, the shim needs a +job whose *name* matches the real check, which for `e2e-tests.yml` would mean +reproducing fourteen shard names and six container shard names. + +So: the trigger-level `paths:` filters are gone, and each expensive workflow +grew a cheap `changes` job that applies the same list one level down. + +``` +changes ──> static_checks ─┐ + └─> test_shards ───┼──> test <- the required check, if: always() + └─> ... ──┘ +``` + +`.github/scripts/changed-paths.py` implements GitHub's filter-pattern semantics +(`*`, `**`, `?`, leading `!`, last match wins) and is unit-tested in +`changed-paths_test.py`, which also asserts the pattern lists still live in the +workflows and still select what they used to. It rejects `+`, `[...]`, and +`{...}` rather than quietly matching them as literals. + +Two properties of this shape matter: + +- **The gate job always runs.** It is `if: always()` and treats a `skipped` + dependency as a pass and a `cancelled` one as a failure. This is on purpose: + it means the required check never depends on how GitHub reports a + conditionally-skipped job, only on a conclusion the workflow computes itself. +- **The gate covers every job in its workflow.** A queue gates on check names an + admin picks by hand, so a job outside the gate's `needs` is a job whose + failure would never block a merge, silently. `backend-tests.yml`'s + `postgres-boot` and `test-windows` were outside its aggregate before this + change and are now inside it. + +Two workflows -- `lint-action-pinning.yml` and `lint-harness-files.yml` -- have +no `changes` job. They are a checkout plus a handful of short linters, so they +simply always run: cheaper than the job that would decide whether to run them, +and it closes the gap where a subject outside their hand-maintained lists +changed and nothing checked it. Their contract tests were updated to assert the +absence of a filter rather than the presence of specific entries. + +### PR context in a merge group + +A `merge_group` run has no `github.event.pull_request` and no +`github.event.before`. Every expression that read one has been made explicit +about the event it is on, and given the merge-group equivalent, +`github.event.merge_group.base_sha` -- the commit the queue branch was cut from, +which is the merge group's base: + +- `architecture-lint.yml` -- the two mutually exclusive `pull_request` / `push` + lint steps were collapsed into one, behind a baseline-resolving step. Under + the old shape a merge-group run would have executed the linter's self-tests + and then linted nothing at all, reporting green having compared the branch + against no baseline. +- `backend-tests.yml` -- `Resolve base SHA`. +- `frontend-tests.yml` -- `Check formatting`, the i18n new-code ratchet, and the + E2E sleep ratchet. +- `e2e-tests.yml` -- the GHCR login guard reads + `github.event_name != 'pull_request' || `, which resolves to the + first operand on a merge group. That is correct: the queue branch lives in + this repository and its token is a base-repository token. + +Concurrency groups were checked too. None of the six keyed on a PR number +(`preview-env.yml` does, and it is not a gate). All six now set +`cancel-in-progress: ${{ github.event_name != 'merge_group' }}`, because a queue +reads a cancelled check as a failure and ejects the pull request. + +## Enabling it + +1. Add a `merge_queue` rule to the `main` ruleset. +2. Add a `required_status_checks` rule and select exactly the six check names in + the table above. Do **not** select any per-shard name (`E2E Shard 3/14`, + `Backend Tests (1/2)`, ...) -- they are covered by the gates, and a queue + configured against them breaks the next time a shard count changes. +3. Do **not** select `lint` (from `pr-title.yml`), `Validate public docs`, or + any `claude-review-*` / `opencode-review-*` / `deploy-*` check. Those never + run in a merge group and the queue will wait on them indefinitely. + +### Caveat: the ruleset can still be bypassed + +The `main` ruleset lists one bypass actor: + +```json +{ "actor_type": "OrganizationAdmin", "bypass_mode": "always" } +``` + +`bypass_mode: always` means an organization admin can merge past whatever is +configured here, queue included, without a review request or a temporary +override. Configuring required checks constrains everybody else; it does not +constrain an org admin. If the point of enabling the queue is that nothing red +reaches `main`, that entry needs to change as well -- to `pull_request` mode, or +removed -- and that is a separate decision about who is trusted to break glass. + +## What is verified, and what is not + +Verified locally on this branch: + +- `actionlint` v1.7.12 is clean on every file touched here. Its one repository + finding is pre-existing and in `release.yml` (`queue: max` under + `concurrency`, a key it does not know). +- `zizmor` v1.29.0, run over the six workflows before and after: one finding + removed (a `template-injection` error in `frontend-tests.yml`, from moving + `github.base_ref` out of an inline expression into `env:`), none added. +- Every `.github/scripts/*_test.py` contract test passes, as does + `lint-action-pinning.py` over all 18 workflows. +- `changed-paths_test.py` passes, and fails when the guarantees are broken: + restoring a `paths:` filter to a gating trigger, deleting a `merge_group` + trigger, and reordering `!**/*.md` above the includes were each confirmed to + fail it. + +Not verified, and not verifiable until a queue is switched on: + +- **No `merge_group` event has ever fired against these workflows.** Everything + about the merge-group path -- that `github.event.merge_group.base_sha` is + populated as expected, that the `changes` job resolves a usable base from it, + that the gate jobs report under the names in the table -- is read from + GitHub's documented payload, not observed. +- Whether GitHub blocks a pull request from *entering* the queue on a check that + reports nothing. The design assumes it does, which is why the filters moved; + if it turns out not to, the change is still correct, just more cautious than + it needed to be. +- The cost. Every pull request now starts six workflows instead of however many + its paths selected, and each merge group runs the full E2E suite. The + `changes` jobs keep the expensive work filtered, but the floor per pull + request is higher than it was. + +The cheapest way to close the first gap is to enable the queue on a low-traffic +day and watch the first merge group's check names against the table above.