From 6ad17c07de7b0a0a8e2747b809dc116dbc6b0e4f Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Mon, 29 Jun 2026 23:10:28 +0200 Subject: [PATCH 1/2] =?UTF-8?q?feat(ci):=20add=20=F0=9F=A7=A9=20Subset=20l?= =?UTF-8?q?abel=20for=20explicit=20role=20selection=20on=20PRs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The maintainer-only manual CI dispatch (workflow_dispatch) cannot be triggered by fork contributors. The 🧩 Subset label closes that gap: when present on a PR, the detect-affected-roles job resolves the deploy whitelist from an explicit roles: YAML block in the PR body instead of the diff, giving fork PRs the same narrowed matrix a maintainer would dispatch manually. Without the label, behavior is unchanged (the dispatcher falls through to the diff resolver, output byte-identical). - subset_roles.py: parse and strictly validate the PR-body roles block (invalid YAML / empty / unknown id / path-traversal all hard-fail) - pr_affected_roles.sh: single dispatcher selecting subset vs diff resolver - entry-pull-request-change.yml: wire the label and PR body into the job - PR templates, pipeline/workflows docs and READMEs, integration tests Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/PULL_REQUEST_TEMPLATE/pipeline.md | 12 +++ .github/PULL_REQUEST_TEMPLATE/server.md | 13 +++ .github/PULL_REQUEST_TEMPLATE/system.md | 12 +++ .github/PULL_REQUEST_TEMPLATE/workstation.md | 12 +++ .../workflows/entry-pull-request-change.yml | 15 +-- docs/contributing/artefact/git/pipeline.md | 30 ++++++ .../tools/github/actions/workflows.md | 2 +- scripts/github/resolve/README.md | 2 +- scripts/github/resolve/pr_affected_roles.sh | 36 +++++++ scripts/meta/resolve/pr/README.md | 3 +- scripts/meta/resolve/pr/subset_roles.py | 99 +++++++++++++++++++ .../meta/workflows/test_subset_roles.py | 98 ++++++++++++++++++ 12 files changed, 320 insertions(+), 14 deletions(-) create mode 100755 scripts/github/resolve/pr_affected_roles.sh create mode 100755 scripts/meta/resolve/pr/subset_roles.py create mode 100644 tests/integration/meta/workflows/test_subset_roles.py diff --git a/.github/PULL_REQUEST_TEMPLATE/pipeline.md b/.github/PULL_REQUEST_TEMPLATE/pipeline.md index 2c03a487ee..138ed609ac 100644 --- a/.github/PULL_REQUEST_TEMPLATE/pipeline.md +++ b/.github/PULL_REQUEST_TEMPLATE/pipeline.md @@ -29,6 +29,18 @@ List the impacted workflows and related automation. --- +## Roles (optional `🧩 Subset` CI scope) + +Optional. Ignored unless a maintainer applies the **🧩 Subset** label; without the label CI uses the diff-derived role set as usual. When the label is set, CI deploys **only** the roles listed here — each must be an existing `roles/` directory, or the run fails. This gives fork PRs the manual-dispatch whitelist. See [pipeline.md](../../docs/contributing/artefact/git/pipeline.md#subset-label-). + +```yaml +roles: + # - web-app-nextcloud + # - sys-version +``` + +--- + ## Change Type Select the semantic version impact of this change: diff --git a/.github/PULL_REQUEST_TEMPLATE/server.md b/.github/PULL_REQUEST_TEMPLATE/server.md index 1bfc6387bd..194c2d4e1a 100644 --- a/.github/PULL_REQUEST_TEMPLATE/server.md +++ b/.github/PULL_REQUEST_TEMPLATE/server.md @@ -39,6 +39,19 @@ Integrate the change into the following services when possible: --- +## Roles (optional `🧩 Subset` CI scope) + +Optional. Ignored unless a maintainer applies the **🧩 Subset** label; without the label CI uses the diff-derived role set as usual. When the label is set, CI deploys **only** the roles listed here — each must be an existing `roles/` directory, or the run fails. This gives fork PRs the manual-dispatch whitelist. See [pipeline.md](../../docs/contributing/artefact/git/pipeline.md#subset-label-). + +```yaml +roles: + # - web-app-nextcloud + # - web-app-matomo + # - sys-version +``` + +--- + ## Change Type Select the semantic version impact of this change: diff --git a/.github/PULL_REQUEST_TEMPLATE/system.md b/.github/PULL_REQUEST_TEMPLATE/system.md index 7d1f0c3a97..88e90c3cc6 100644 --- a/.github/PULL_REQUEST_TEMPLATE/system.md +++ b/.github/PULL_REQUEST_TEMPLATE/system.md @@ -30,6 +30,18 @@ List the impacted roles and shared components. --- +## Roles (optional `🧩 Subset` CI scope) + +Optional. Ignored unless a maintainer applies the **🧩 Subset** label; without the label CI uses the diff-derived role set as usual. When the label is set, CI deploys **only** the roles listed here — each must be an existing `roles/` directory, or the run fails. This gives fork PRs the manual-dispatch whitelist. See [pipeline.md](../../docs/contributing/artefact/git/pipeline.md#subset-label-). + +```yaml +roles: + # - sys-version + # - svc-db-postgres +``` + +--- + ## Change Type Select the semantic version impact of this change: diff --git a/.github/PULL_REQUEST_TEMPLATE/workstation.md b/.github/PULL_REQUEST_TEMPLATE/workstation.md index 026a02a9b4..50013057c0 100644 --- a/.github/PULL_REQUEST_TEMPLATE/workstation.md +++ b/.github/PULL_REQUEST_TEMPLATE/workstation.md @@ -30,6 +30,18 @@ List the impacted roles and workstation context. --- +## Roles (optional `🧩 Subset` CI scope) + +Optional. Ignored unless a maintainer applies the **🧩 Subset** label; without the label CI uses the diff-derived role set as usual. When the label is set, CI deploys **only** the roles listed here — each must be an existing `roles/` directory, or the run fails. This gives fork PRs the manual-dispatch whitelist. See [pipeline.md](../../docs/contributing/artefact/git/pipeline.md#subset-label-). + +```yaml +roles: + # - desk-firefox + # - sys-version +``` + +--- + ## Change Type Select the semantic version impact of this change: diff --git a/.github/workflows/entry-pull-request-change.yml b/.github/workflows/entry-pull-request-change.yml index 8e033ef679..76d7457740 100644 --- a/.github/workflows/entry-pull-request-change.yml +++ b/.github/workflows/entry-pull-request-change.yml @@ -107,17 +107,10 @@ jobs: - name: Resolve affected roles id: resolve shell: bash - run: | - set -euo pipefail - resolved="$(./scripts/meta/resolve/diff/affected_roles.sh)" - echo "Resolver output: ${resolved}" - if [[ "${resolved}" == "__ALL__" ]]; then - echo "whitelist=__ALL__" >>"${GITHUB_OUTPUT}" - echo "roles_only=false" >>"${GITHUB_OUTPUT}" - else - echo "whitelist=${resolved}" >>"${GITHUB_OUTPUT}" - echo "roles_only=true" >>"${GITHUB_OUTPUT}" - fi + env: + SUBSET_LABELED: ${{ contains(github.event.pull_request.labels.*.name, '🧩 Subset') }} + PR_BODY: ${{ github.event.pull_request.body }} + run: bash scripts/github/resolve/pr_affected_roles.sh ci-orchestrator: needs: diff --git a/docs/contributing/artefact/git/pipeline.md b/docs/contributing/artefact/git/pipeline.md index 8ad064fee5..886bacc1f9 100644 --- a/docs/contributing/artefact/git/pipeline.md +++ b/docs/contributing/artefact/git/pipeline.md @@ -86,6 +86,36 @@ The PR-scope short-circuits in [scope.sh](../../../../scripts/meta/resolve/pr/sc The reverse closure is implemented in [affected resolver](../../../../cli/meta/roles/applications/resolution/affected/__main__.py) and invoked from [affected_roles.sh](../../../../scripts/meta/resolve/diff/affected_roles.sh). The workflow glue lives in [effective_whitelist.sh](../../../../scripts/github/resolve/effective_whitelist.sh). [test-deploy-local.yml](../../../../.github/workflows/test-deploy-local.yml) MUST NOT apply this resolution. Local dispatch keeps the explicit whitelist semantics. +#### Subset label 🧩 + +The manual whitelist (`entry-manual.yml`, `workflow_dispatch`) is only reachable by maintainers on the upstream repository; **fork contributors cannot dispatch it**. The `🧩 Subset` label closes that gap: it lets a PR pin CI to an explicit role list declared in the PR body, so a fork PR can run the same narrowed matrix a maintainer would have dispatched manually. + +The mechanism is **opt-in and fully backwards-compatible**. Without the `🧩 Subset` label nothing changes: the diff-driven precedence above applies exactly as before. The label is what activates the subset; the role list alone does nothing. + +The `detect-affected-roles` job in [entry-pull-request-change.yml](../../../../.github/workflows/entry-pull-request-change.yml) always calls the single dispatcher [pr_affected_roles.sh](../../../../scripts/github/resolve/pr_affected_roles.sh), which branches on the label: with `🧩 Subset` it runs [subset_roles.py](../../../../scripts/meta/resolve/pr/subset_roles.py) **instead of** the diff resolver, otherwise it falls through to the unchanged diff path. The subset script reads the PR body and expects a fenced YAML block with a `roles:` list, conventionally under a `## Roles` heading: + +````markdown +## Roles + +```yaml +roles: + - web-app-nextcloud + - web-app-matomo + - sys-version +``` +```` + +The resolved list becomes the `whitelist` output (with `roles_only=true`), which flows through the orchestrator into the deploy tests as precedence rung 2 above (explicit whitelist wins over the diff). The first fenced block whose YAML carries a `roles:` key is used, so the heading text is not significant. + +Unlike the diff resolver (which fail-safe widens to `__ALL__` on any problem), the subset path is **strict** and fails the run with a clear message when: + +- the YAML is invalid, +- no `roles:` block is found, +- the `roles:` list is empty, or +- any listed id is not an existing `roles/` directory. + +This prevents a mistyped subset from silently deploying the wrong set. The `🧩 Subset` label, like `🛡️ Trusted`, must exist in the repository's label set and is applied through the PR labels UI; both the `pull_request` and `pull_request_target` events carry a `labeled` trigger, so adding it starts a fresh run that observes the subset. Because applying labels requires triage/write access, in practice a maintainer applies it after the contributor has filled in the `## Roles` block. + ### 10. Installation Tests 📦 The install-test workflows listed in the `Infrastructure tests` table of [workflows.md](../../tools/github/actions/workflows.md) run in parallel once `code-quality-gate` is green. All of them MUST pass `test-install-gate`. diff --git a/docs/contributing/tools/github/actions/workflows.md b/docs/contributing/tools/github/actions/workflows.md index ee4e5dda10..2818bbf234 100644 --- a/docs/contributing/tools/github/actions/workflows.md +++ b/docs/contributing/tools/github/actions/workflows.md @@ -25,7 +25,7 @@ Trigger column legend: **auto** = fires automatically (push, pull_request, sched | Workflow | Description | Trigger | Inputs | |---|---|---|---| -| [entry-pull-request-change.yml](../../../../../.github/workflows/entry-pull-request-change.yml): `⚡ CI: Pull Request` | Detects PR scope from changed files + branch prefix, then conditionally calls the orchestrator. A maintainer `🛡️ Trusted` label builds the privileged images from the PR head — see [pipeline.md](../../../artefact/git/pipeline.md#trusted-fork-prs-). | auto (`pull_request` & `pull_request_target`: opened, synchronize, reopened, ready_for_review, labeled) | none | +| [entry-pull-request-change.yml](../../../../../.github/workflows/entry-pull-request-change.yml): `⚡ CI: Pull Request` | Detects PR scope from changed files + branch prefix, then conditionally calls the orchestrator. A maintainer `🛡️ Trusted` label builds the privileged images from the PR head — see [pipeline.md](../../../artefact/git/pipeline.md#trusted-fork-prs-). A `🧩 Subset` label pins the deploy matrix to the roles listed in the PR body (giving fork PRs the manual-dispatch whitelist) — see [pipeline.md](../../../artefact/git/pipeline.md#subset-label-). | auto (`pull_request` & `pull_request_target`: opened, synchronize, reopened, ready_for_review, labeled) | none | | [entry-push-latest.yml](../../../../../.github/workflows/entry-push-latest.yml): `⚡ CI: Push` | Syncs `main` from the configured source repository, then runs the orchestrator on pushes to `main`, `feature/**`, `hotfix/**`, `fix/**`, `chore/**`, `alert-autofix-*`; on tagged `main` it also calls `release-version.yml`. Respects the `CI_CANCEL_IN_PROGRESS`, `CI_SYNC_MAIN_SOURCE_REPOSITORY`, and `CI_RUN_ON_MAIN` repository variables, see [configuration.md](configuration.md). | auto (`push`) | none | | [entry-manual.yml](../../../../../.github/workflows/entry-manual.yml): `⚡ CI: Manual` | Manual dispatch of the full orchestrator for a chosen distro set and app whitelist. The `whitelist` input accepts the `__ALL__` sentinel for forced full deploy; see the "Diff-driven app selection" subsection in [pipeline.md](../../../artefact/git/pipeline.md). | manual | `distros` (default `debian`), `whitelist` (optional; `__ALL__` forces full deploy, empty triggers diff derivation, any other value is verbatim) | | [entry-pull-request-cancel.yml](../../../../../.github/workflows/entry-pull-request-cancel.yml): `🚫 Cancel: PR Runs on Close` | Cancels active workflow runs for a PR when it is closed or converted to draft. | auto (`pull_request_target`: closed, converted_to_draft) | none | diff --git a/scripts/github/resolve/README.md b/scripts/github/resolve/README.md index 4e80d9320d..d5da3ed504 100644 --- a/scripts/github/resolve/README.md +++ b/scripts/github/resolve/README.md @@ -4,6 +4,6 @@ Shell helpers that derive structured inputs and outputs for GitHub Actions jobs ## Scope 📋 -This directory contains pure resolvers that read repository state (git history, role metadata, version tags, input environment variables) and emit resolved values either via `$GITHUB_OUTPUT` or stdout. Resolvers MUST be idempotent and MUST NOT mutate repository or registry state beyond writing to `$GITHUB_OUTPUT`. The covered concerns are distro selection for push CI, diff-driven app whitelist resolution, release-tag parsing, app discovery for the deploy matrix, and change detection that gates the update pipeline. +This directory contains pure resolvers that read repository state (git history, role metadata, version tags, input environment variables) and emit resolved values either via `$GITHUB_OUTPUT` or stdout. Resolvers MUST be idempotent and MUST NOT mutate repository or registry state beyond writing to `$GITHUB_OUTPUT`. The covered concerns are distro selection for push CI, diff-driven app whitelist resolution, the `🧩 Subset` label dispatch (`pr_affected_roles.sh`, which picks between the PR-body subset and the diff resolver), release-tag parsing, app discovery for the deploy matrix, and change detection that gates the update pipeline. For the workflow catalog that consumes these outputs see [workflows.md](../../../docs/contributing/tools/github/actions/workflows.md). For the diff-driven whitelist contract see the "Diff-driven app selection" section in [pipeline.md](../../../docs/contributing/artefact/git/pipeline.md). diff --git a/scripts/github/resolve/pr_affected_roles.sh b/scripts/github/resolve/pr_affected_roles.sh new file mode 100755 index 0000000000..0ae2c3be2c --- /dev/null +++ b/scripts/github/resolve/pr_affected_roles.sh @@ -0,0 +1,36 @@ +#!/usr/bin/env bash +# +# Resolve the role whitelist for a pull-request CI run and write +# `whitelist` + `roles_only` to GITHUB_OUTPUT. This is the single +# entry point the `detect-affected-roles` job calls; it keeps the +# subset-vs-diff selection logic out of the workflow YAML. +# +# Inputs via env: +# SUBSET_LABELED "true" when the PR carries the '🧩 Subset' label. +# When set, the explicit role list declared in the PR +# body wins, resolved by +# scripts/meta/resolve/pr/subset_roles.py (strict: it +# fails the run on invalid/empty/unknown roles). +# Otherwise the diff-derived resolver runs unchanged. +# PR_BODY PR body markdown (only read on the subset path). +# +# Outputs (GITHUB_OUTPUT): +# whitelist= +# roles_only= + +set -euo pipefail + +: "${GITHUB_OUTPUT:?Missing GITHUB_OUTPUT}" + +if [[ "${SUBSET_LABELED:-}" == "true" ]]; then + echo "🧩 Subset label present: restricting CI to the roles listed in the PR body." + exec python scripts/meta/resolve/pr/subset_roles.py +fi + +resolved="$(./scripts/meta/resolve/diff/affected_roles.sh)" +echo "Resolver output: ${resolved}" +if [[ "${resolved}" == "__ALL__" ]]; then + printf 'whitelist=__ALL__\nroles_only=false\n' >>"${GITHUB_OUTPUT}" +else + printf 'whitelist=%s\nroles_only=true\n' "${resolved}" >>"${GITHUB_OUTPUT}" +fi diff --git a/scripts/meta/resolve/pr/README.md b/scripts/meta/resolve/pr/README.md index 3ef4e65008..1e7b0c78bd 100644 --- a/scripts/meta/resolve/pr/README.md +++ b/scripts/meta/resolve/pr/README.md @@ -7,5 +7,6 @@ Examples in this folder: - `scope.sh` resolves whether a Pull Request is agents-only, documentation-only, or full scope - `branch_prefix.sh` validates that the PR branch prefix matches the detected scope; see [branch.md](../../../../docs/contributing/artefact/git/branch.md) for the authoritative list of valid prefixes and their CI impact - `merge_ref.sh` resolves the merge ref used for forked PR workflows +- `subset_roles.py` parses the `roles:` block from the PR body when the `🧩 Subset` label is set, validates the ids against `roles/`, and emits the restricted whitelist; see the "Subset label" section in [pipeline.md](../../../../docs/contributing/artefact/git/pipeline.md) -These scripts keep PR scope detection, branch-prefix validation, and fork merge-ref resolution together in one place. +These scripts keep PR scope detection, branch-prefix validation, fork merge-ref resolution, and subset-label parsing together in one place. diff --git a/scripts/meta/resolve/pr/subset_roles.py b/scripts/meta/resolve/pr/subset_roles.py new file mode 100755 index 0000000000..92d97d0bcd --- /dev/null +++ b/scripts/meta/resolve/pr/subset_roles.py @@ -0,0 +1,99 @@ +#!/usr/bin/env python3 +"""Resolve the explicit role subset declared in a pull-request body. + +Activated by the ``🧩 Subset`` label: the entry-pull-request workflow only +runs this script when the label is present. It gives fork contributors the +selective-CI capability that ``entry-manual.yml`` (``workflow_dispatch``) +grants maintainers, which forks cannot trigger. + +It reads the PR body from ``$PR_BODY``, extracts the first fenced code block +whose YAML defines a ``roles:`` list, validates that every listed id is an +existing ``roles/`` directory, and writes ``whitelist`` (space-separated) +and ``roles_only=true`` to ``$GITHUB_OUTPUT`` -- the same outputs the +diff-derived resolver produces. + +Unlike the fail-safe diff resolver (which widens to ``__ALL__`` on any +problem), the subset path is strict: invalid YAML, a missing/empty ``roles:`` +list, or an unknown role id each abort with a clear message and a non-zero +exit, so a mistyped subset never silently deploys the wrong set. +""" + +import os +import re +import sys +from pathlib import Path +from typing import NoReturn + +import yaml + +REPO_ROOT = Path(__file__).resolve().parents[4] +ROLES_DIR = REPO_ROOT / "roles" +FENCE_RE = re.compile(r"```[^\n]*\n(.*?)```", re.DOTALL) +# Role dir names are lowercase alphanumerics + dashes. Enforcing this on the +# untrusted PR body also blocks path traversal (no '/', no '.') before any +# filesystem lookup. +NAME_RE = re.compile(r"^[a-z0-9-]+$") + + +def fail(message: str) -> NoReturn: + print(f"Subset roles error: {message}", file=sys.stderr) + sys.exit(1) + + +def extract_roles(body: str): + blocks = FENCE_RE.findall(body) + if not blocks: + fail( + "the '🧩 Subset' label is set but the PR body has no fenced " + "```yaml block. Add a '## Roles' section with a `roles:` list." + ) + for block in blocks: + try: + data = yaml.safe_load(block) + except yaml.YAMLError as exc: + fail(f"invalid YAML in the PR body roles block: {exc}") + if isinstance(data, dict) and "roles" in data: + return data["roles"] + fail( + "no `roles:` key found in any YAML block of the PR body. Declare the " + "subset as a `roles:` list under a '## Roles' section." + ) + + +def main() -> None: + roles = extract_roles(os.environ.get("PR_BODY", "")) + if roles is None: + roles = [] + if not isinstance(roles, list) or not all(isinstance(r, str) for r in roles): + fail("`roles:` must be a YAML list of role-id strings.") + + roles = [r.strip() for r in roles if r and r.strip()] + if not roles: + fail( + "the '🧩 Subset' label is set but the `roles:` list is empty. " + "List at least one role id or remove the label." + ) + + invalid = sorted({r for r in roles if not NAME_RE.fullmatch(r)}) + if invalid: + fail( + "invalid role id(s) (allowed: lowercase letters, digits, '-'): " + + ", ".join(invalid) + ) + + unknown = sorted({r for r in roles if not (ROLES_DIR / r).is_dir()}) + if unknown: + fail("unknown role id(s): " + ", ".join(unknown)) + + whitelist = " ".join(roles) + print(f"Subset roles resolved from PR body: {whitelist}") + + github_output = os.environ.get("GITHUB_OUTPUT") + if github_output: + with Path(github_output).open("a", encoding="utf-8") as handle: + handle.write(f"whitelist={whitelist}\n") + handle.write("roles_only=true\n") + + +if __name__ == "__main__": + main() diff --git a/tests/integration/meta/workflows/test_subset_roles.py b/tests/integration/meta/workflows/test_subset_roles.py new file mode 100644 index 0000000000..6677dfe230 --- /dev/null +++ b/tests/integration/meta/workflows/test_subset_roles.py @@ -0,0 +1,98 @@ +import os +import shutil +import subprocess +import tempfile +import unittest +from pathlib import Path + +from . import PROJECT_ROOT + +SCRIPT_REL = Path("scripts/meta/resolve/pr/subset_roles.py") +SCRIPT_PATH = PROJECT_ROOT / SCRIPT_REL + + +class TestSubsetRoles(unittest.TestCase): + """Exercises the '🧩 Subset' resolver. + + The "no subset label" case (existing diff behaviour stays unchanged) is + covered by test_diff_affected_roles.py: this script only ever runs when + the label gates it in entry-pull-request-change.yml. + """ + + def _run(self, *, body: str, roles: tuple[str, ...] = ("web-app-foo",)): + tmp = Path(tempfile.mkdtemp(prefix="subset-roles-")) + self.addCleanup(shutil.rmtree, tmp, ignore_errors=True) + + # Plant the real script at its expected relative path so REPO_ROOT + # (parents[4]) lands inside the temp tree, and create the roles/ dirs + # the validation checks against. + script_target = tmp / SCRIPT_REL + script_target.parent.mkdir(parents=True) + shutil.copy2(SCRIPT_PATH, script_target) + for role in roles: + (tmp / "roles" / role).mkdir(parents=True) + + output_file = tmp / "output.txt" + output_file.touch() + + env = os.environ.copy() + env["PR_BODY"] = body + env["GITHUB_OUTPUT"] = str(output_file) + + result = subprocess.run( + ["python", str(script_target)], + env=env, + capture_output=True, + text=True, + check=False, + ) + + outputs = {} + for line in output_file.read_text(encoding="utf-8").splitlines(): + if "=" in line: + key, value = line.split("=", 1) + outputs[key] = value + return result, outputs + + def test_valid_roles_produce_whitelist(self): + body = "## Roles\n\n```yaml\nroles:\n - web-app-foo\n - web-app-bar\n```\n" + result, outputs = self._run(body=body, roles=("web-app-foo", "web-app-bar")) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(outputs["whitelist"], "web-app-foo web-app-bar") + self.assertEqual(outputs["roles_only"], "true") + + def test_invalid_yaml_fails(self): + body = "```yaml\nroles:\n - web-app-foo\n bad: : :\n```\n" + result, outputs = self._run(body=body) + self.assertNotEqual(result.returncode, 0) + self.assertIn("invalid YAML", result.stderr) + self.assertNotIn("whitelist", outputs) + + def test_unknown_role_fails(self): + body = "```yaml\nroles:\n - web-app-foo\n - web-app-ghost\n```\n" + result, outputs = self._run(body=body, roles=("web-app-foo",)) + self.assertNotEqual(result.returncode, 0) + self.assertIn("web-app-ghost", result.stderr) + self.assertNotIn("whitelist", outputs) + + def test_path_traversal_id_is_rejected(self): + body = "```yaml\nroles:\n - ../../etc\n```\n" + result, outputs = self._run(body=body) + self.assertNotEqual(result.returncode, 0) + self.assertIn("invalid role id", result.stderr) + self.assertNotIn("whitelist", outputs) + + def test_empty_role_list_fails(self): + body = "```yaml\nroles:\n```\n" + result, _ = self._run(body=body) + self.assertNotEqual(result.returncode, 0) + self.assertIn("empty", result.stderr) + + def test_missing_block_fails(self): + result, _ = self._run(body="No machine-readable roles block here.\n") + self.assertNotEqual(result.returncode, 0) + self.assertIn("no fenced", result.stderr) + + +if __name__ == "__main__": + unittest.main() From 0bcd63b658274ea2bcdee1d36a998701166bd0dc Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Mon, 29 Jun 2026 23:54:56 +0200 Subject: [PATCH 2/2] =?UTF-8?q?fix(ci):=20make=20=F0=9F=A7=A9=20Subset=20r?= =?UTF-8?q?esolver=20comply=20with=20repo=20lint=20rules?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The merged subset feature failed test-lint: a .py under scripts/, a local parents[N] PROJECT_ROOT, a direct yaml.safe_load, an uncached read, and narration comments. - move scripts/meta/resolve/pr/subset_roles.py -> cli.meta.ci.subset_roles (scripts/ is .sh-only); dispatcher invokes it via python -m - parse via utils.cache.yaml.load_yaml_str; import utils.PROJECT_ROOT - test invokes the module via -m against real roles; nocheck the tempfile read - strip narration comments; docs document HOW not WHY Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/PULL_REQUEST_TEMPLATE/pipeline.md | 2 +- .github/PULL_REQUEST_TEMPLATE/server.md | 2 +- .github/PULL_REQUEST_TEMPLATE/system.md | 2 +- .github/PULL_REQUEST_TEMPLATE/workstation.md | 2 +- .../pr => cli/meta/ci}/subset_roles.py | 21 +++-- docs/contributing/artefact/git/pipeline.md | 12 +-- .../tools/github/actions/workflows.md | 2 +- scripts/github/resolve/pr_affected_roles.sh | 8 +- scripts/meta/resolve/pr/README.md | 3 +- .../meta/workflows/test_subset_roles.py | 87 +++++++++---------- 10 files changed, 66 insertions(+), 75 deletions(-) rename {scripts/meta/resolve/pr => cli/meta/ci}/subset_roles.py (88%) mode change 100755 => 100644 diff --git a/.github/PULL_REQUEST_TEMPLATE/pipeline.md b/.github/PULL_REQUEST_TEMPLATE/pipeline.md index 138ed609ac..0b08822498 100644 --- a/.github/PULL_REQUEST_TEMPLATE/pipeline.md +++ b/.github/PULL_REQUEST_TEMPLATE/pipeline.md @@ -31,7 +31,7 @@ List the impacted workflows and related automation. ## Roles (optional `🧩 Subset` CI scope) -Optional. Ignored unless a maintainer applies the **🧩 Subset** label; without the label CI uses the diff-derived role set as usual. When the label is set, CI deploys **only** the roles listed here — each must be an existing `roles/` directory, or the run fails. This gives fork PRs the manual-dispatch whitelist. See [pipeline.md](../../docs/contributing/artefact/git/pipeline.md#subset-label-). +Optional. Ignored unless a maintainer applies the **🧩 Subset** label; without the label CI uses the diff-derived role set as usual. When the label is set, CI deploys **only** the roles listed here — each must be an existing `roles/` directory, or the run fails. See [pipeline.md](../../docs/contributing/artefact/git/pipeline.md#subset-label-). ```yaml roles: diff --git a/.github/PULL_REQUEST_TEMPLATE/server.md b/.github/PULL_REQUEST_TEMPLATE/server.md index 194c2d4e1a..6828cb6f45 100644 --- a/.github/PULL_REQUEST_TEMPLATE/server.md +++ b/.github/PULL_REQUEST_TEMPLATE/server.md @@ -41,7 +41,7 @@ Integrate the change into the following services when possible: ## Roles (optional `🧩 Subset` CI scope) -Optional. Ignored unless a maintainer applies the **🧩 Subset** label; without the label CI uses the diff-derived role set as usual. When the label is set, CI deploys **only** the roles listed here — each must be an existing `roles/` directory, or the run fails. This gives fork PRs the manual-dispatch whitelist. See [pipeline.md](../../docs/contributing/artefact/git/pipeline.md#subset-label-). +Optional. Ignored unless a maintainer applies the **🧩 Subset** label; without the label CI uses the diff-derived role set as usual. When the label is set, CI deploys **only** the roles listed here — each must be an existing `roles/` directory, or the run fails. See [pipeline.md](../../docs/contributing/artefact/git/pipeline.md#subset-label-). ```yaml roles: diff --git a/.github/PULL_REQUEST_TEMPLATE/system.md b/.github/PULL_REQUEST_TEMPLATE/system.md index 88e90c3cc6..d4b4881f9a 100644 --- a/.github/PULL_REQUEST_TEMPLATE/system.md +++ b/.github/PULL_REQUEST_TEMPLATE/system.md @@ -32,7 +32,7 @@ List the impacted roles and shared components. ## Roles (optional `🧩 Subset` CI scope) -Optional. Ignored unless a maintainer applies the **🧩 Subset** label; without the label CI uses the diff-derived role set as usual. When the label is set, CI deploys **only** the roles listed here — each must be an existing `roles/` directory, or the run fails. This gives fork PRs the manual-dispatch whitelist. See [pipeline.md](../../docs/contributing/artefact/git/pipeline.md#subset-label-). +Optional. Ignored unless a maintainer applies the **🧩 Subset** label; without the label CI uses the diff-derived role set as usual. When the label is set, CI deploys **only** the roles listed here — each must be an existing `roles/` directory, or the run fails. See [pipeline.md](../../docs/contributing/artefact/git/pipeline.md#subset-label-). ```yaml roles: diff --git a/.github/PULL_REQUEST_TEMPLATE/workstation.md b/.github/PULL_REQUEST_TEMPLATE/workstation.md index 50013057c0..fc0d189fac 100644 --- a/.github/PULL_REQUEST_TEMPLATE/workstation.md +++ b/.github/PULL_REQUEST_TEMPLATE/workstation.md @@ -32,7 +32,7 @@ List the impacted roles and workstation context. ## Roles (optional `🧩 Subset` CI scope) -Optional. Ignored unless a maintainer applies the **🧩 Subset** label; without the label CI uses the diff-derived role set as usual. When the label is set, CI deploys **only** the roles listed here — each must be an existing `roles/` directory, or the run fails. This gives fork PRs the manual-dispatch whitelist. See [pipeline.md](../../docs/contributing/artefact/git/pipeline.md#subset-label-). +Optional. Ignored unless a maintainer applies the **🧩 Subset** label; without the label CI uses the diff-derived role set as usual. When the label is set, CI deploys **only** the roles listed here — each must be an existing `roles/` directory, or the run fails. See [pipeline.md](../../docs/contributing/artefact/git/pipeline.md#subset-label-). ```yaml roles: diff --git a/scripts/meta/resolve/pr/subset_roles.py b/cli/meta/ci/subset_roles.py old mode 100755 new mode 100644 similarity index 88% rename from scripts/meta/resolve/pr/subset_roles.py rename to cli/meta/ci/subset_roles.py index 92d97d0bcd..86a494cd9c --- a/scripts/meta/resolve/pr/subset_roles.py +++ b/cli/meta/ci/subset_roles.py @@ -1,8 +1,7 @@ -#!/usr/bin/env python3 """Resolve the explicit role subset declared in a pull-request body. Activated by the ``🧩 Subset`` label: the entry-pull-request workflow only -runs this script when the label is present. It gives fork contributors the +runs this module when the label is present. It gives fork contributors the selective-CI capability that ``entry-manual.yml`` (``workflow_dispatch``) grants maintainers, which forks cannot trigger. @@ -15,7 +14,12 @@ diff-derived resolver produces. Unlike the fail-safe diff resolver (which widens to ``__ALL__`` on any problem), the subset path is strict: invalid YAML, a missing/empty ``roles:`` list, or an unknown role id each abort with a clear message and a non-zero -exit, so a mistyped subset never silently deploys the wrong set. +exit, so a mistyped subset never silently deploys the wrong set. Role ids must +match ``^[a-z0-9-]+$``, which also blocks path traversal before any filesystem +lookup. + +Invoked from scripts/github/resolve/pr_affected_roles.sh via +``python -m cli.meta.ci.subset_roles``. """ import os @@ -26,12 +30,11 @@ from typing import NoReturn import yaml -REPO_ROOT = Path(__file__).resolve().parents[4] -ROLES_DIR = REPO_ROOT / "roles" +from utils import PROJECT_ROOT +from utils.cache.yaml import load_yaml_str + +ROLES_DIR = PROJECT_ROOT / "roles" FENCE_RE = re.compile(r"```[^\n]*\n(.*?)```", re.DOTALL) -# Role dir names are lowercase alphanumerics + dashes. Enforcing this on the -# untrusted PR body also blocks path traversal (no '/', no '.') before any -# filesystem lookup. NAME_RE = re.compile(r"^[a-z0-9-]+$") @@ -49,7 +52,7 @@ def extract_roles(body: str): ) for block in blocks: try: - data = yaml.safe_load(block) + data = load_yaml_str(block) except yaml.YAMLError as exc: fail(f"invalid YAML in the PR body roles block: {exc}") if isinstance(data, dict) and "roles" in data: diff --git a/docs/contributing/artefact/git/pipeline.md b/docs/contributing/artefact/git/pipeline.md index 886bacc1f9..a986f7f1a1 100644 --- a/docs/contributing/artefact/git/pipeline.md +++ b/docs/contributing/artefact/git/pipeline.md @@ -88,11 +88,7 @@ The reverse closure is implemented in [affected resolver](../../../../cli/meta/r #### Subset label 🧩 -The manual whitelist (`entry-manual.yml`, `workflow_dispatch`) is only reachable by maintainers on the upstream repository; **fork contributors cannot dispatch it**. The `🧩 Subset` label closes that gap: it lets a PR pin CI to an explicit role list declared in the PR body, so a fork PR can run the same narrowed matrix a maintainer would have dispatched manually. - -The mechanism is **opt-in and fully backwards-compatible**. Without the `🧩 Subset` label nothing changes: the diff-driven precedence above applies exactly as before. The label is what activates the subset; the role list alone does nothing. - -The `detect-affected-roles` job in [entry-pull-request-change.yml](../../../../.github/workflows/entry-pull-request-change.yml) always calls the single dispatcher [pr_affected_roles.sh](../../../../scripts/github/resolve/pr_affected_roles.sh), which branches on the label: with `🧩 Subset` it runs [subset_roles.py](../../../../scripts/meta/resolve/pr/subset_roles.py) **instead of** the diff resolver, otherwise it falls through to the unchanged diff path. The subset script reads the PR body and expects a fenced YAML block with a `roles:` list, conventionally under a `## Roles` heading: +Add a `## Roles` section with a fenced `roles:` YAML list to the PR body and apply the `🧩 Subset` label: ````markdown ## Roles @@ -105,16 +101,16 @@ roles: ``` ```` -The resolved list becomes the `whitelist` output (with `roles_only=true`), which flows through the orchestrator into the deploy tests as precedence rung 2 above (explicit whitelist wins over the diff). The first fenced block whose YAML carries a `roles:` key is used, so the heading text is not significant. +With the label set, the `detect-affected-roles` job in [entry-pull-request-change.yml](../../../../.github/workflows/entry-pull-request-change.yml) calls [pr_affected_roles.sh](../../../../scripts/github/resolve/pr_affected_roles.sh), which runs the [cli.meta.ci.subset_roles](../../../../cli/meta/ci/subset_roles.py) module. The module reads the first fenced block whose YAML carries a `roles:` key (the heading text is not matched) and emits that list as the `whitelist` output with `roles_only=true`, which flows into the deploy tests as precedence rung 2 above. Without the label, the diff-driven precedence applies. -Unlike the diff resolver (which fail-safe widens to `__ALL__` on any problem), the subset path is **strict** and fails the run with a clear message when: +The subset path fails the run when: - the YAML is invalid, - no `roles:` block is found, - the `roles:` list is empty, or - any listed id is not an existing `roles/` directory. -This prevents a mistyped subset from silently deploying the wrong set. The `🧩 Subset` label, like `🛡️ Trusted`, must exist in the repository's label set and is applied through the PR labels UI; both the `pull_request` and `pull_request_target` events carry a `labeled` trigger, so adding it starts a fresh run that observes the subset. Because applying labels requires triage/write access, in practice a maintainer applies it after the contributor has filled in the `## Roles` block. +The `🧩 Subset` label must exist in the repository's label set and is applied through the PR labels UI. Both the `pull_request` and `pull_request_target` events carry a `labeled` trigger, so adding it starts a fresh run. ### 10. Installation Tests 📦 diff --git a/docs/contributing/tools/github/actions/workflows.md b/docs/contributing/tools/github/actions/workflows.md index 2818bbf234..2b20ea6545 100644 --- a/docs/contributing/tools/github/actions/workflows.md +++ b/docs/contributing/tools/github/actions/workflows.md @@ -25,7 +25,7 @@ Trigger column legend: **auto** = fires automatically (push, pull_request, sched | Workflow | Description | Trigger | Inputs | |---|---|---|---| -| [entry-pull-request-change.yml](../../../../../.github/workflows/entry-pull-request-change.yml): `⚡ CI: Pull Request` | Detects PR scope from changed files + branch prefix, then conditionally calls the orchestrator. A maintainer `🛡️ Trusted` label builds the privileged images from the PR head — see [pipeline.md](../../../artefact/git/pipeline.md#trusted-fork-prs-). A `🧩 Subset` label pins the deploy matrix to the roles listed in the PR body (giving fork PRs the manual-dispatch whitelist) — see [pipeline.md](../../../artefact/git/pipeline.md#subset-label-). | auto (`pull_request` & `pull_request_target`: opened, synchronize, reopened, ready_for_review, labeled) | none | +| [entry-pull-request-change.yml](../../../../../.github/workflows/entry-pull-request-change.yml): `⚡ CI: Pull Request` | Detects PR scope from changed files + branch prefix, then conditionally calls the orchestrator. A maintainer `🛡️ Trusted` label builds the privileged images from the PR head — see [pipeline.md](../../../artefact/git/pipeline.md#trusted-fork-prs-). A `🧩 Subset` label pins the deploy matrix to the roles listed in the PR body — see [pipeline.md](../../../artefact/git/pipeline.md#subset-label-). | auto (`pull_request` & `pull_request_target`: opened, synchronize, reopened, ready_for_review, labeled) | none | | [entry-push-latest.yml](../../../../../.github/workflows/entry-push-latest.yml): `⚡ CI: Push` | Syncs `main` from the configured source repository, then runs the orchestrator on pushes to `main`, `feature/**`, `hotfix/**`, `fix/**`, `chore/**`, `alert-autofix-*`; on tagged `main` it also calls `release-version.yml`. Respects the `CI_CANCEL_IN_PROGRESS`, `CI_SYNC_MAIN_SOURCE_REPOSITORY`, and `CI_RUN_ON_MAIN` repository variables, see [configuration.md](configuration.md). | auto (`push`) | none | | [entry-manual.yml](../../../../../.github/workflows/entry-manual.yml): `⚡ CI: Manual` | Manual dispatch of the full orchestrator for a chosen distro set and app whitelist. The `whitelist` input accepts the `__ALL__` sentinel for forced full deploy; see the "Diff-driven app selection" subsection in [pipeline.md](../../../artefact/git/pipeline.md). | manual | `distros` (default `debian`), `whitelist` (optional; `__ALL__` forces full deploy, empty triggers diff derivation, any other value is verbatim) | | [entry-pull-request-cancel.yml](../../../../../.github/workflows/entry-pull-request-cancel.yml): `🚫 Cancel: PR Runs on Close` | Cancels active workflow runs for a PR when it is closed or converted to draft. | auto (`pull_request_target`: closed, converted_to_draft) | none | diff --git a/scripts/github/resolve/pr_affected_roles.sh b/scripts/github/resolve/pr_affected_roles.sh index 0ae2c3be2c..bdbab69c0d 100755 --- a/scripts/github/resolve/pr_affected_roles.sh +++ b/scripts/github/resolve/pr_affected_roles.sh @@ -8,9 +8,9 @@ # Inputs via env: # SUBSET_LABELED "true" when the PR carries the '🧩 Subset' label. # When set, the explicit role list declared in the PR -# body wins, resolved by -# scripts/meta/resolve/pr/subset_roles.py (strict: it -# fails the run on invalid/empty/unknown roles). +# body wins, resolved by the cli.meta.ci.subset_roles +# module (strict: it fails the run on +# invalid/empty/unknown roles). # Otherwise the diff-derived resolver runs unchanged. # PR_BODY PR body markdown (only read on the subset path). # @@ -24,7 +24,7 @@ set -euo pipefail if [[ "${SUBSET_LABELED:-}" == "true" ]]; then echo "🧩 Subset label present: restricting CI to the roles listed in the PR body." - exec python scripts/meta/resolve/pr/subset_roles.py + exec python -m cli.meta.ci.subset_roles fi resolved="$(./scripts/meta/resolve/diff/affected_roles.sh)" diff --git a/scripts/meta/resolve/pr/README.md b/scripts/meta/resolve/pr/README.md index 1e7b0c78bd..cbdd5c3bf3 100644 --- a/scripts/meta/resolve/pr/README.md +++ b/scripts/meta/resolve/pr/README.md @@ -7,6 +7,5 @@ Examples in this folder: - `scope.sh` resolves whether a Pull Request is agents-only, documentation-only, or full scope - `branch_prefix.sh` validates that the PR branch prefix matches the detected scope; see [branch.md](../../../../docs/contributing/artefact/git/branch.md) for the authoritative list of valid prefixes and their CI impact - `merge_ref.sh` resolves the merge ref used for forked PR workflows -- `subset_roles.py` parses the `roles:` block from the PR body when the `🧩 Subset` label is set, validates the ids against `roles/`, and emits the restricted whitelist; see the "Subset label" section in [pipeline.md](../../../../docs/contributing/artefact/git/pipeline.md) -These scripts keep PR scope detection, branch-prefix validation, fork merge-ref resolution, and subset-label parsing together in one place. +These scripts keep PR scope detection, branch-prefix validation, and fork merge-ref resolution together in one place. The `🧩 Subset` label's PR-body parser lives in the `cli.meta.ci.subset_roles` module; the dispatcher [pr_affected_roles.sh](../../../github/resolve/pr_affected_roles.sh) selects between it and the diff resolver. See the "Subset label" section in [pipeline.md](../../../../docs/contributing/artefact/git/pipeline.md). diff --git a/tests/integration/meta/workflows/test_subset_roles.py b/tests/integration/meta/workflows/test_subset_roles.py index 6677dfe230..a6eb36a682 100644 --- a/tests/integration/meta/workflows/test_subset_roles.py +++ b/tests/integration/meta/workflows/test_subset_roles.py @@ -1,5 +1,4 @@ import os -import shutil import subprocess import tempfile import unittest @@ -7,89 +6,83 @@ from pathlib import Path from . import PROJECT_ROOT -SCRIPT_REL = Path("scripts/meta/resolve/pr/subset_roles.py") -SCRIPT_PATH = PROJECT_ROOT / SCRIPT_REL +REAL_ROLES = ("web-app-nextcloud", "web-app-matomo") class TestSubsetRoles(unittest.TestCase): - """Exercises the '🧩 Subset' resolver. + """Exercises the cli.meta.ci.subset_roles resolver behind the '🧩 Subset' + label. The "no subset label" case (existing diff behaviour stays unchanged) is - covered by test_diff_affected_roles.py: this script only ever runs when + covered by test_diff_affected_roles.py: this module only ever runs when the label gates it in entry-pull-request-change.yml. """ - def _run(self, *, body: str, roles: tuple[str, ...] = ("web-app-foo",)): - tmp = Path(tempfile.mkdtemp(prefix="subset-roles-")) - self.addCleanup(shutil.rmtree, tmp, ignore_errors=True) + def _run(self, body: str): + with tempfile.TemporaryDirectory() as tmp: + output_file = Path(tmp) / "output.txt" + output_file.touch() - # Plant the real script at its expected relative path so REPO_ROOT - # (parents[4]) lands inside the temp tree, and create the roles/ dirs - # the validation checks against. - script_target = tmp / SCRIPT_REL - script_target.parent.mkdir(parents=True) - shutil.copy2(SCRIPT_PATH, script_target) - for role in roles: - (tmp / "roles" / role).mkdir(parents=True) + env = os.environ.copy() + env["PR_BODY"] = body + env["GITHUB_OUTPUT"] = str(output_file) - output_file = tmp / "output.txt" - output_file.touch() + result = subprocess.run( + ["python", "-m", "cli.meta.ci.subset_roles"], + cwd=PROJECT_ROOT, + env=env, + capture_output=True, + text=True, + check=False, + ) - env = os.environ.copy() - env["PR_BODY"] = body - env["GITHUB_OUTPUT"] = str(output_file) - - result = subprocess.run( - ["python", str(script_target)], - env=env, - capture_output=True, - text=True, - check=False, - ) - - outputs = {} - for line in output_file.read_text(encoding="utf-8").splitlines(): - if "=" in line: - key, value = line.split("=", 1) - outputs[key] = value - return result, outputs + outputs = {} + raw = output_file.read_text(encoding="utf-8") # nocheck: cache-read + for line in raw.splitlines(): + if "=" in line: + key, value = line.split("=", 1) + outputs[key] = value + return result, outputs def test_valid_roles_produce_whitelist(self): - body = "## Roles\n\n```yaml\nroles:\n - web-app-foo\n - web-app-bar\n```\n" - result, outputs = self._run(body=body, roles=("web-app-foo", "web-app-bar")) + body = ( + "## Roles\n\n```yaml\nroles:\n" + f" - {REAL_ROLES[0]}\n - {REAL_ROLES[1]}\n```\n" + ) + result, outputs = self._run(body) self.assertEqual(result.returncode, 0, result.stderr) - self.assertEqual(outputs["whitelist"], "web-app-foo web-app-bar") + self.assertEqual(outputs["whitelist"], " ".join(REAL_ROLES)) self.assertEqual(outputs["roles_only"], "true") def test_invalid_yaml_fails(self): - body = "```yaml\nroles:\n - web-app-foo\n bad: : :\n```\n" - result, outputs = self._run(body=body) + body = "```yaml\nroles:\n - web-app-nextcloud\n bad: : :\n```\n" + result, outputs = self._run(body) self.assertNotEqual(result.returncode, 0) self.assertIn("invalid YAML", result.stderr) self.assertNotIn("whitelist", outputs) def test_unknown_role_fails(self): - body = "```yaml\nroles:\n - web-app-foo\n - web-app-ghost\n```\n" - result, outputs = self._run(body=body, roles=("web-app-foo",)) + body = "```yaml\nroles:\n - web-app-nextcloud\n - web-app-ghost-xyz\n```\n" + result, outputs = self._run(body) self.assertNotEqual(result.returncode, 0) - self.assertIn("web-app-ghost", result.stderr) + self.assertIn("web-app-ghost-xyz", result.stderr) self.assertNotIn("whitelist", outputs) def test_path_traversal_id_is_rejected(self): body = "```yaml\nroles:\n - ../../etc\n```\n" - result, outputs = self._run(body=body) + result, outputs = self._run(body) self.assertNotEqual(result.returncode, 0) self.assertIn("invalid role id", result.stderr) self.assertNotIn("whitelist", outputs) def test_empty_role_list_fails(self): body = "```yaml\nroles:\n```\n" - result, _ = self._run(body=body) + result, _ = self._run(body) self.assertNotEqual(result.returncode, 0) self.assertIn("empty", result.stderr) def test_missing_block_fails(self): - result, _ = self._run(body="No machine-readable roles block here.\n") + result, _ = self._run("No machine-readable roles block here.\n") self.assertNotEqual(result.returncode, 0) self.assertIn("no fenced", result.stderr)