From e482a6fb82eecfb0581491574b65ad1ea5f8ead4 Mon Sep 17 00:00:00 2001 From: "thomas.boyer.chammard" Date: Thu, 12 Nov 2026 01:52:48 +0000 Subject: [PATCH 1/5] ci: run the manifest entries a PR adds as "(proposed)" External CI checks on that PR --- .github/actions/ext-ci/README.md | 18 +++- .github/actions/ext-ci/orchestrator.py | 139 ++++++++++++++++++++++--- 2 files changed, 140 insertions(+), 17 deletions(-) diff --git a/.github/actions/ext-ci/README.md b/.github/actions/ext-ci/README.md index d472c8db42d..3712d98d4e6 100644 --- a/.github/actions/ext-ci/README.md +++ b/.github/actions/ext-ci/README.md @@ -19,11 +19,23 @@ also runs for merge groups and on manual dispatch (`repos`, `pr_number` or `fpri | `repos[].default_ref` | branch to dispatch when neither `pr-` nor a branch named like the F´ base branch exists there | | `repos[].timeout_minutes` | remote running time allowed, and separately the time it may wait in queue; the remote run is cancelled on timeout. Keep `2 * timeout_minutes` under the job's `timeout-minutes` in ext-ci.yml | | `repos[].failure_policy` | `block`: a failure is a red check and fails the orchestrator job; `notify`: neutral (non-blocking) check | -| `repos[].on_pr` | `false`: never dispatched for a PR head (merge queue and manual `fprime_ref` runs only) | +| `repos[].on_pr` | `false`: never dispatched for a PR head (merge queue and manual `fprime_ref` runs only); not honoured for the PR that adds the entry (see below) | Orchestrator-side faults (dispatch error, run never started, polling lost the run) fail the job whatever the policy. A re-run of the orchestrator re-dispatches only the repos whose check is not green. +### Proposed entries (a PR that edits manifest.json) + +A PR that adds an entry, or re-points an existing one to another `repository`/`workflow`, gets that +entry dispatched on its own checks as `External CI / (proposed)`: `failure_policy: notify`, never +required, `on_pr` ignored. The PR's manifest is author-controlled data, so the default-branch manifest +stays authoritative for everything it defines (`check_prefix`, `paths_ignore`, existing ids: a PR +cannot redefine an existing check, the devel entry still runs under its normal name). A remote run held +by a protected environment shows as "Waiting for approval"; approve it within the orchestrator job's +`timeout-minutes` or it is cancelled. The repository must already be in the External CI App installation +or the proposed check reports `dispatch failed ... 404`. An unreadable PR manifest gives a single failed +`External CI / manifest (proposed)` check. + ## Contract for an external `ci.yml` - `on: workflow_dispatch` with inputs `fprime_repository`, `fprime_ref` (exact SHA to use as the F´ @@ -31,7 +43,9 @@ whatever the policy. A re-run of the orchestrator re-dispatches only the repos w - `permissions: contents: read` (or `{}`), no `${{ secrets.* }}`, no Actions cache writes on dispatched runs: the dispatched run executes untrusted F´ PR code in the external repository's own context. - Repositories with self-hosted or hardware runners: `on_pr: false` in the manifest, ephemeral runners, - SHA-pinned actions, and a job-level guard on `inputs.fprime_repository` in the workflow. + SHA-pinned actions, a protected environment with required reviewers on the hardware job, and a job-level + guard on `inputs.fprime_repository` in the workflow. The manifest alone is not the gate: the PR that adds + the entry dispatches it once (see "Proposed entries"). ## Credentials diff --git a/.github/actions/ext-ci/orchestrator.py b/.github/actions/ext-ci/orchestrator.py index 3d8bf300307..f07fae3e733 100644 --- a/.github/actions/ext-ci/orchestrator.py +++ b/.github/actions/ext-ci/orchestrator.py @@ -19,6 +19,7 @@ remote runs """ +import base64 import json import os import re @@ -37,6 +38,7 @@ STATE_FILE = "ext-ci-state.json" RESULTS_FILE = "ext-ci-results.json" MANIFEST_PATH = ".github/actions/ext-ci/manifest.json" +PROPOSED = " (proposed)" # id suffix of entries taken from the PR's own manifest POLL_S = int(env.get("POLL_INTERVAL_SECONDS") or 60) ORCHESTRATOR_FAULTS = ( "dispatch_failed", @@ -315,23 +317,71 @@ def relevant_change(files: list[str] | None, manifest: dict) -> bool: return not all(ignored(f) for f in files) -def select_repos(manifest: dict, target: dict) -> list[dict]: - """Return the manifest entries to run: all of them, narrowed to the `repos` input ids on a manual run - (unknown id raises RuntimeError) and to entries with `on_pr` true (the default) when a PR is tested, - manual runs included.""" - repos = manifest["repos"] +def select_repos(manifest: dict, target: dict, proposed: list[dict] = ()) -> list[dict]: + """Return the manifest entries to run: all of them plus the PR's `proposed` ones, narrowed to the + `repos` input ids on a manual run (unknown id raises RuntimeError; a proposed entry matches its + base id) and to entries with `on_pr` true (the default) when a PR is tested, manual runs included. + Proposed entries ignore `on_pr`: the PR introducing an entry is the one run it must get. + """ + repos = manifest["repos"] + list(proposed) wanted = (env.get("INPUT_REPOS") or "all").strip() if EVENT_NAME == "workflow_dispatch" and wanted and wanted != "all": ids = [s.strip() for s in wanted.split(",") if s.strip()] - unknown = [i for i in ids if not any(entry["id"] == i for entry in repos)] + base_id = lambda entry: entry["id"].removesuffix(PROPOSED) # noqa: E731 + unknown = [i for i in ids if not any(base_id(entry) == i for entry in repos)] if unknown: raise RuntimeError("unknown manifest ids: " + ", ".join(unknown)) - repos = [entry for entry in repos if entry["id"] in ids] + repos = [entry for entry in repos if base_id(entry) in ids] if target["pr"]: - repos = [entry for entry in repos if entry.get("on_pr", True)] + repos = [ + entry + for entry in repos + if entry.get("on_pr", True) or entry.get("proposed") + ] return repos +def proposed_repos(manifest: dict, target: dict, files: list[str] | None) -> list[dict]: + """Entries the PR under test adds to manifest.json (or re-points to another repository/workflow), + so the PR that introduces an external repo exercises it on its own checks. + + The PR's manifest is PR-author-controlled data, so the default-branch manifest stays authoritative + for everything it defines: `check_prefix`, `paths_ignore` and every existing id (a PR cannot + redefine, and thereby trivially pass, an existing check). Proposed entries run under + ` (proposed)` with `failure_policy: notify`; `on_pr` is not honoured for them, the external + workflow's own guard (environment, `fprime_repository` check) is what protects hardware runners. + Raises on an unreadable or malformed PR manifest; the caller reports that as a failed check. + """ + if not target["pr"] or not files or MANIFEST_PATH not in files: + return [] + blob = own.get( + f"/repos/{target['head_repo']}/contents/{MANIFEST_PATH}?ref={target['head_sha']}" + ) + head = json.loads(base64.b64decode(blob["content"])) + current = {e["id"]: e for e in manifest["repos"]} + proposed = [] + for e in head["repos"]: + known = current.get(e["id"]) + if known and (known["repository"], known["workflow"]) == ( + e["repository"], + e["workflow"], + ): + continue + if not re.fullmatch(r"[\w.-]+/[\w.-]+", e["repository"]) or not re.fullmatch( + r"[\w.-]+\.ya?ml", e["workflow"] + ): + raise ValueError(f"entry {e['id']!r}: malformed repository or workflow") + proposed.append( + { + **e, + "id": e["id"] + PROPOSED, + "failure_policy": "notify", + "proposed": True, + } + ) + return proposed + + def resolve_ext_ref(entry: dict, target: dict) -> str: """Return the external repo branch to dispatch: its `pr-` branch if it exists, else a branch named like the F´ base branch (`devel`, `release/x`), else the entry's `default_ref` (`main`). @@ -383,6 +433,7 @@ def dispatch_one(manifest: dict, entry: dict, target: dict, state: dict) -> None "failure_policy": entry.get("failure_policy", "block"), "timeout_minutes": entry.get("timeout_minutes", 60), "dispatched_at": now().strftime("%Y-%m-%dT%H:%M:%SZ"), + "proposed": bool(entry.get("proposed")), "check_run_id": None, "remote_run_id": None, "remote_run_url": None, @@ -450,7 +501,15 @@ def dispatch_one(manifest: dict, entry: dict, target: dict, state: dict) -> None except ( Exception ) as e: # noqa: BLE001 - one repo's failure must not stop the fan-out - s["error"], s["remote_conclusion"] = f"dispatch failed: {e}", "dispatch_failed" + hint = ( + " (no such workflow, or the repository is not in the External CI App installation)" + if isinstance(e, ApiError) and e.status == 404 + else "" + ) + s["error"], s["remote_conclusion"] = ( + f"dispatch failed: {e}{hint}", + "dispatch_failed", + ) log(f"::error::[{entry['id']}] {s['error']}") save_state(state) @@ -463,6 +522,24 @@ def cancel_run(s: dict) -> None: log(f"::warning::[{s['id']}] cancel: {e}") +def report_waiting(s: dict) -> None: + """Retitle one repo's Check Run while its remote run waits for an environment approval.""" + log(f"[{s['id']}] waiting for approval in {s['repository']}: {s['remote_run_url']}") + try: + own.patch( + f"/repos/{REPO}/check-runs/{s['check_run_id']}", + { + "output": { + "title": f"Waiting for approval in {s['repository']}", + "summary": f"The remote run is held by a protected environment and needs a reviewer " + f"of {code(s['repository'])}: {s['remote_run_url']}\n\nOrchestrator: {ORCHESTRATOR_URL}", + }, + }, + ) + except ApiError as e: # cosmetic + log(f"::warning::[{s['id']}] check update: {e}") + + def poll_all(state: dict) -> None: """Poll every dispatched run every POLL_S seconds until each has a `remote_conclusion`. @@ -484,8 +561,17 @@ def active() -> list[dict]: run = ext.get( f"/repos/{s['repository']}/actions/runs/{s['remote_run_id']}" ) - if run.get("run_started_at") and not s["remote_started_at"]: - s["remote_started_at"] = run["run_started_at"] + if run["status"] == "waiting": + # Held for a protected-environment approval in the external repo: neither clock + # runs; the orchestrator job's own timeout bounds the wait (finalize cancels it). + if not s.get("waiting_reported"): + s["waiting_reported"] = True + report_waiting(s) + continue + if run["status"] == "in_progress" and not s["remote_started_at"]: + # Observed start, not `run_started_at`: GitHub stamps that at creation, approval + # wait included. + s["remote_started_at"] = now().strftime("%Y-%m-%dT%H:%M:%SZ") timeout = timedelta(minutes=s["timeout_minutes"]) if run["status"] == "completed": s["remote_conclusion"] = run["conclusion"] @@ -691,7 +777,15 @@ def run() -> None: "Use workflow_dispatch to run it here deliberately." ) return - repos = select_repos(manifest, target) + files = changed_files(target) + bad_manifest = None + try: + proposed = proposed_repos(manifest, target, files) + except ( + Exception + ) as e: # noqa: BLE001 - a broken PR manifest gets its own failed check below + proposed, bad_manifest = [], f"{type(e).__name__}: {e}" + repos = select_repos(manifest, target, proposed) if ATTEMPT > 1: # re-run: only the repos whose check is not green yet repos = [e for e in repos if not check_succeeded(manifest, e, target)] log( @@ -699,9 +793,22 @@ def run() -> None: ) state = {"target": target, "attempt": ATTEMPT, "repos": {}} save_state(state) - if EVENT_NAME != "workflow_dispatch" and not relevant_change( - changed_files(target), manifest - ): + if bad_manifest: + own.post( + f"/repos/{REPO}/check-runs", + { + "name": check_name(manifest, "manifest" + PROPOSED), + "head_sha": target["head_sha"], + "status": "completed", + "conclusion": "failure", + "details_url": ORCHESTRATOR_URL, + "output": { + "title": "This PR's manifest.json could not be used", + "summary": f"{code(bad_manifest)}\n\nOrchestrator: {ORCHESTRATOR_URL}", + }, + }, + ) + if EVENT_NAME != "workflow_dispatch" and not relevant_change(files, manifest): log("no relevant files changed; marking all repos as skipped") skip_all(manifest, target, repos, state) write_summary(target, state) @@ -724,6 +831,8 @@ def run() -> None: s["id"] for s in state["repos"].values() if (s["conclusion"] or check_conclusion(s)) != "success" + # a PR's own (proposed) entry never fails the job: App not installed yet, ... + and not s.get("proposed") and ( s["failure_policy"] != "notify" or s["remote_conclusion"] in ORCHESTRATOR_FAULTS From 5386fbd69d0bbe1d73ed104103ee59a36c7d758b Mon Sep 17 00:00:00 2001 From: "thomas.boyer.chammard" Date: Thu, 12 Nov 2026 01:56:21 +0000 Subject: [PATCH 2/5] ext-ci: validate ids and timeout_minutes of a PR's proposed manifest entries --- .github/actions/ext-ci/orchestrator.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/.github/actions/ext-ci/orchestrator.py b/.github/actions/ext-ci/orchestrator.py index f07fae3e733..f6b3913d36a 100644 --- a/.github/actions/ext-ci/orchestrator.py +++ b/.github/actions/ext-ci/orchestrator.py @@ -359,8 +359,17 @@ def proposed_repos(manifest: dict, target: dict, files: list[str] | None) -> lis ) head = json.loads(base64.b64decode(blob["content"])) current = {e["id"]: e for e in manifest["repos"]} + ids = [e.get("id") for e in head["repos"]] + if len(set(ids)) != len(ids): + raise ValueError("duplicate ids") proposed = [] for e in head["repos"]: + if not isinstance(e.get("id"), str) or not isinstance( + e.get("timeout_minutes", 60), int + ): + raise ValueError( + f"entry {e.get('id')!r}: id must be a string, timeout_minutes an integer" + ) known = current.get(e["id"]) if known and (known["repository"], known["workflow"]) == ( e["repository"], From e2f11a871ac9b2f3a2d53069708ef29dad33909d Mon Sep 17 00:00:00 2001 From: "thomas.boyer.chammard" Date: Thu, 12 Nov 2026 01:56:21 +0000 Subject: [PATCH 3/5] ext-ci: restart the timeout clocks after a protected-environment wait --- .github/actions/ext-ci/orchestrator.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/.github/actions/ext-ci/orchestrator.py b/.github/actions/ext-ci/orchestrator.py index f6b3913d36a..a24c0673342 100644 --- a/.github/actions/ext-ci/orchestrator.py +++ b/.github/actions/ext-ci/orchestrator.py @@ -571,8 +571,11 @@ def active() -> list[dict]: f"/repos/{s['repository']}/actions/runs/{s['remote_run_id']}" ) if run["status"] == "waiting": - # Held for a protected-environment approval in the external repo: neither clock - # runs; the orchestrator job's own timeout bounds the wait (finalize cancels it). + # Held for a protected-environment approval in the external repo (possibly after + # earlier jobs ran): both clocks restart when it is released; the orchestrator + # job's own timeout bounds the wait (finalize cancels it). + s["remote_started_at"] = None + s["dispatched_at"] = now().strftime("%Y-%m-%dT%H:%M:%SZ") if not s.get("waiting_reported"): s["waiting_reported"] = True report_waiting(s) From a0a11d1348fc05844e5883a020f8bbdac82d3ecb Mon Sep 17 00:00:00 2001 From: "thomas.boyer.chammard" Date: Thu, 12 Nov 2026 02:35:28 +0000 Subject: [PATCH 4/5] ext-ci: run from the PR's manifest.json as is; drop the "(proposed)" labelling --- .github/actions/ext-ci/README.md | 23 +++--- .github/actions/ext-ci/orchestrator.py | 104 ++++++------------------- 2 files changed, 32 insertions(+), 95 deletions(-) diff --git a/.github/actions/ext-ci/README.md b/.github/actions/ext-ci/README.md index 3712d98d4e6..85d830d1a35 100644 --- a/.github/actions/ext-ci/README.md +++ b/.github/actions/ext-ci/README.md @@ -19,22 +19,19 @@ also runs for merge groups and on manual dispatch (`repos`, `pr_number` or `fpri | `repos[].default_ref` | branch to dispatch when neither `pr-` nor a branch named like the F´ base branch exists there | | `repos[].timeout_minutes` | remote running time allowed, and separately the time it may wait in queue; the remote run is cancelled on timeout. Keep `2 * timeout_minutes` under the job's `timeout-minutes` in ext-ci.yml | | `repos[].failure_policy` | `block`: a failure is a red check and fails the orchestrator job; `notify`: neutral (non-blocking) check | -| `repos[].on_pr` | `false`: never dispatched for a PR head (merge queue and manual `fprime_ref` runs only); not honoured for the PR that adds the entry (see below) | +| `repos[].on_pr` | `false`: never dispatched for a PR head (merge queue and manual `fprime_ref` runs only); ignored for an entry on the PR that adds it (see below) | Orchestrator-side faults (dispatch error, run never started, polling lost the run) fail the job whatever the policy. A re-run of the orchestrator re-dispatches only the repos whose check is not green. -### Proposed entries (a PR that edits manifest.json) +### A PR that edits manifest.json -A PR that adds an entry, or re-points an existing one to another `repository`/`workflow`, gets that -entry dispatched on its own checks as `External CI / (proposed)`: `failure_policy: notify`, never -required, `on_pr` ignored. The PR's manifest is author-controlled data, so the default-branch manifest -stays authoritative for everything it defines (`check_prefix`, `paths_ignore`, existing ids: a PR -cannot redefine an existing check, the devel entry still runs under its normal name). A remote run held -by a protected environment shows as "Waiting for approval"; approve it within the orchestrator job's -`timeout-minutes` or it is cancelled. The repository must already be in the External CI App installation -or the proposed check reports `dispatch failed ... 404`. An unreadable PR manifest gives a single failed -`External CI / manifest (proposed)` check. +The orchestrator then runs from the PR head's `manifest.json` (fetched as data; the workflow and the +script still come from the default branch), so a PR adding an external repository exercises it on its +own checks. Entries the default branch does not know ignore `on_pr`. The repository must already be in +the External CI App installation or the check reports `dispatch failed ... 404`. A remote run held by a +protected environment shows as "Waiting for approval"; both timeout clocks restart when it is released +and the orchestrator job's `timeout-minutes` bounds the wait. ## Contract for an external `ci.yml` @@ -44,8 +41,8 @@ or the proposed check reports `dispatch failed ... 404`. An unreadable PR manife runs: the dispatched run executes untrusted F´ PR code in the external repository's own context. - Repositories with self-hosted or hardware runners: `on_pr: false` in the manifest, ephemeral runners, SHA-pinned actions, a protected environment with required reviewers on the hardware job, and a job-level - guard on `inputs.fprime_repository` in the workflow. The manifest alone is not the gate: the PR that adds - the entry dispatches it once (see "Proposed entries"). + guard on `inputs.fprime_repository` in the workflow. `on_pr` alone is not the gate: the PR that adds the + entry dispatches it once. ## Credentials diff --git a/.github/actions/ext-ci/orchestrator.py b/.github/actions/ext-ci/orchestrator.py index a24c0673342..d3c0c611435 100644 --- a/.github/actions/ext-ci/orchestrator.py +++ b/.github/actions/ext-ci/orchestrator.py @@ -38,7 +38,6 @@ STATE_FILE = "ext-ci-state.json" RESULTS_FILE = "ext-ci-results.json" MANIFEST_PATH = ".github/actions/ext-ci/manifest.json" -PROPOSED = " (proposed)" # id suffix of entries taken from the PR's own manifest POLL_S = int(env.get("POLL_INTERVAL_SECONDS") or 60) ORCHESTRATOR_FAULTS = ( "dispatch_failed", @@ -317,78 +316,43 @@ def relevant_change(files: list[str] | None, manifest: dict) -> bool: return not all(ignored(f) for f in files) -def select_repos(manifest: dict, target: dict, proposed: list[dict] = ()) -> list[dict]: - """Return the manifest entries to run: all of them plus the PR's `proposed` ones, narrowed to the - `repos` input ids on a manual run (unknown id raises RuntimeError; a proposed entry matches its - base id) and to entries with `on_pr` true (the default) when a PR is tested, manual runs included. - Proposed entries ignore `on_pr`: the PR introducing an entry is the one run it must get. - """ - repos = manifest["repos"] + list(proposed) +def select_repos(manifest: dict, target: dict) -> list[dict]: + """Return the manifest entries to run: all of them, narrowed to the `repos` input ids on a manual run + (unknown id raises RuntimeError) and to entries with `on_pr` true (the default) when a PR is tested, + manual runs included. Entries the PR itself adds (`new`, see pr_manifest) ignore `on_pr`: the PR + introducing an entry is the one run it must get.""" + repos = manifest["repos"] wanted = (env.get("INPUT_REPOS") or "all").strip() if EVENT_NAME == "workflow_dispatch" and wanted and wanted != "all": ids = [s.strip() for s in wanted.split(",") if s.strip()] - base_id = lambda entry: entry["id"].removesuffix(PROPOSED) # noqa: E731 - unknown = [i for i in ids if not any(base_id(entry) == i for entry in repos)] + unknown = [i for i in ids if not any(entry["id"] == i for entry in repos)] if unknown: raise RuntimeError("unknown manifest ids: " + ", ".join(unknown)) - repos = [entry for entry in repos if base_id(entry) in ids] + repos = [entry for entry in repos if entry["id"] in ids] if target["pr"]: repos = [ - entry - for entry in repos - if entry.get("on_pr", True) or entry.get("proposed") + entry for entry in repos if entry.get("on_pr", True) or entry.get("new") ] return repos -def proposed_repos(manifest: dict, target: dict, files: list[str] | None) -> list[dict]: - """Entries the PR under test adds to manifest.json (or re-points to another repository/workflow), - so the PR that introduces an external repo exercises it on its own checks. - - The PR's manifest is PR-author-controlled data, so the default-branch manifest stays authoritative - for everything it defines: `check_prefix`, `paths_ignore` and every existing id (a PR cannot - redefine, and thereby trivially pass, an existing check). Proposed entries run under - ` (proposed)` with `failure_policy: notify`; `on_pr` is not honoured for them, the external - workflow's own guard (environment, `fprime_repository` check) is what protects hardware runners. - Raises on an unreadable or malformed PR manifest; the caller reports that as a failed check. +def pr_manifest(manifest: dict, target: dict, files: list[str] | None) -> dict: + """The manifest to run from: the PR head's when the PR under test edits manifest.json, so a PR + adding an external repo exercises it on its own checks; the default branch's otherwise. Only the + data is taken from the PR (the workflow and this script stay default-branch); entries whose id the + default branch does not know are marked `new`. Raises when the PR's copy is unreadable. """ if not target["pr"] or not files or MANIFEST_PATH not in files: - return [] + return manifest blob = own.get( f"/repos/{target['head_repo']}/contents/{MANIFEST_PATH}?ref={target['head_sha']}" ) head = json.loads(base64.b64decode(blob["content"])) - current = {e["id"]: e for e in manifest["repos"]} - ids = [e.get("id") for e in head["repos"]] - if len(set(ids)) != len(ids): - raise ValueError("duplicate ids") - proposed = [] - for e in head["repos"]: - if not isinstance(e.get("id"), str) or not isinstance( - e.get("timeout_minutes", 60), int - ): - raise ValueError( - f"entry {e.get('id')!r}: id must be a string, timeout_minutes an integer" - ) - known = current.get(e["id"]) - if known and (known["repository"], known["workflow"]) == ( - e["repository"], - e["workflow"], - ): - continue - if not re.fullmatch(r"[\w.-]+/[\w.-]+", e["repository"]) or not re.fullmatch( - r"[\w.-]+\.ya?ml", e["workflow"] - ): - raise ValueError(f"entry {e['id']!r}: malformed repository or workflow") - proposed.append( - { - **e, - "id": e["id"] + PROPOSED, - "failure_policy": "notify", - "proposed": True, - } - ) - return proposed + known = {entry["id"] for entry in manifest["repos"]} + for entry in head["repos"]: + entry["new"] = entry["id"] not in known + log(f"using the PR's manifest.json ({target['head_sha'][:7]})") + return head def resolve_ext_ref(entry: dict, target: dict) -> str: @@ -442,7 +406,6 @@ def dispatch_one(manifest: dict, entry: dict, target: dict, state: dict) -> None "failure_policy": entry.get("failure_policy", "block"), "timeout_minutes": entry.get("timeout_minutes", 60), "dispatched_at": now().strftime("%Y-%m-%dT%H:%M:%SZ"), - "proposed": bool(entry.get("proposed")), "check_run_id": None, "remote_run_id": None, "remote_run_url": None, @@ -790,14 +753,8 @@ def run() -> None: ) return files = changed_files(target) - bad_manifest = None - try: - proposed = proposed_repos(manifest, target, files) - except ( - Exception - ) as e: # noqa: BLE001 - a broken PR manifest gets its own failed check below - proposed, bad_manifest = [], f"{type(e).__name__}: {e}" - repos = select_repos(manifest, target, proposed) + manifest = pr_manifest(manifest, target, files) + repos = select_repos(manifest, target) if ATTEMPT > 1: # re-run: only the repos whose check is not green yet repos = [e for e in repos if not check_succeeded(manifest, e, target)] log( @@ -805,21 +762,6 @@ def run() -> None: ) state = {"target": target, "attempt": ATTEMPT, "repos": {}} save_state(state) - if bad_manifest: - own.post( - f"/repos/{REPO}/check-runs", - { - "name": check_name(manifest, "manifest" + PROPOSED), - "head_sha": target["head_sha"], - "status": "completed", - "conclusion": "failure", - "details_url": ORCHESTRATOR_URL, - "output": { - "title": "This PR's manifest.json could not be used", - "summary": f"{code(bad_manifest)}\n\nOrchestrator: {ORCHESTRATOR_URL}", - }, - }, - ) if EVENT_NAME != "workflow_dispatch" and not relevant_change(files, manifest): log("no relevant files changed; marking all repos as skipped") skip_all(manifest, target, repos, state) @@ -843,8 +785,6 @@ def run() -> None: s["id"] for s in state["repos"].values() if (s["conclusion"] or check_conclusion(s)) != "success" - # a PR's own (proposed) entry never fails the job: App not installed yet, ... - and not s.get("proposed") and ( s["failure_policy"] != "notify" or s["remote_conclusion"] in ORCHESTRATOR_FAULTS From 744cf360ba0539fc3bb110ef70e541c6306e87d4 Mon Sep 17 00:00:00 2001 From: "thomas.boyer.chammard" Date: Thu, 12 Nov 2026 03:07:58 +0000 Subject: [PATCH 5/5] ext-ci: the revision under test dictates manifest.json; post real conclusions for notify repos MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - head_manifest(): always read manifest.json from the F´ revision under test (PR head, merge-queue commit or manual fprime_ref) through this repository's contents API; the default branch's copy only serves the upstream_repository preflight and the baseline for `new` entries. `new` is only set when the PR itself edits manifest.json, so a stale branch does not resurrect a removed entry. Duplicate ids and a missing file fail early with a clear error. - check_conclusion()/finalize(): failure_policy no longer maps failures to `neutral` (GitHub folds neutral checks away in the merge box and the Checks sidebar); notify repos show the real conclusion and only keep the orchestrator job green. - dispatch hint also on 403 (observed when the repo was not yet in the App installation). --- .github/actions/ext-ci/README.md | 16 +++--- .github/actions/ext-ci/orchestrator.py | 67 ++++++++++++++++---------- 2 files changed, 50 insertions(+), 33 deletions(-) diff --git a/.github/actions/ext-ci/README.md b/.github/actions/ext-ci/README.md index 85d830d1a35..836c4ba9697 100644 --- a/.github/actions/ext-ci/README.md +++ b/.github/actions/ext-ci/README.md @@ -18,18 +18,20 @@ also runs for merge groups and on manual dispatch (`repos`, `pr_number` or `fpri | `repos[].workflow` | workflow file in that repository; must accept the inputs below | | `repos[].default_ref` | branch to dispatch when neither `pr-` nor a branch named like the F´ base branch exists there | | `repos[].timeout_minutes` | remote running time allowed, and separately the time it may wait in queue; the remote run is cancelled on timeout. Keep `2 * timeout_minutes` under the job's `timeout-minutes` in ext-ci.yml | -| `repos[].failure_policy` | `block`: a failure is a red check and fails the orchestrator job; `notify`: neutral (non-blocking) check | -| `repos[].on_pr` | `false`: never dispatched for a PR head (merge queue and manual `fprime_ref` runs only); ignored for an entry on the PR that adds it (see below) | +| `repos[].failure_policy` | `block`: a failure is a red check and fails the orchestrator job; `notify`: the check still shows the real result (red on failure) but the orchestrator job stays green; do not make it a required check | +| `repos[].on_pr` | `false`: never dispatched for a PR head (merge queue and manual `fprime_ref` runs only), except once on the PR that adds the entry (see below) | Orchestrator-side faults (dispatch error, run never started, polling lost the run) fail the job whatever the policy. A re-run of the orchestrator re-dispatches only the repos whose check is not green. -### A PR that edits manifest.json +### Which manifest.json is used -The orchestrator then runs from the PR head's `manifest.json` (fetched as data; the workflow and the -script still come from the default branch), so a PR adding an external repository exercises it on its -own checks. Entries the default branch does not know ignore `on_pr`. The repository must already be in -the External CI App installation or the check reports `dispatch failed ... 404`. A remote run held by a +The one at the F´ revision under test: the PR head, the merge-queue commit or the manual `fprime_ref`. +It is read as data through this repository's contents API (PR head commits are reachable here, fork or +not); the workflow and the script come from the default branch, whose `manifest.json` only serves the +`upstream_repository` preflight. So a PR that adds or edits an entry exercises it on its own checks, and +entries the default branch does not have ignore `on_pr`. The external repository must already be in the +External CI App installation or the check reports `dispatch failed ... 403`. A remote run held by a protected environment shows as "Waiting for approval"; both timeout clocks restart when it is released and the orchestrator job's `timeout-minutes` bounds the wait. diff --git a/.github/actions/ext-ci/orchestrator.py b/.github/actions/ext-ci/orchestrator.py index d3c0c611435..47710c2aeaa 100644 --- a/.github/actions/ext-ci/orchestrator.py +++ b/.github/actions/ext-ci/orchestrator.py @@ -319,7 +319,7 @@ def relevant_change(files: list[str] | None, manifest: dict) -> bool: def select_repos(manifest: dict, target: dict) -> list[dict]: """Return the manifest entries to run: all of them, narrowed to the `repos` input ids on a manual run (unknown id raises RuntimeError) and to entries with `on_pr` true (the default) when a PR is tested, - manual runs included. Entries the PR itself adds (`new`, see pr_manifest) ignore `on_pr`: the PR + manual runs included. Entries the PR itself adds (`new`, see head_manifest) ignore `on_pr`: the PR introducing an entry is the one run it must get.""" repos = manifest["repos"] wanted = (env.get("INPUT_REPOS") or "all").strip() @@ -336,22 +336,37 @@ def select_repos(manifest: dict, target: dict) -> list[dict]: return repos -def pr_manifest(manifest: dict, target: dict, files: list[str] | None) -> dict: - """The manifest to run from: the PR head's when the PR under test edits manifest.json, so a PR - adding an external repo exercises it on its own checks; the default branch's otherwise. Only the - data is taken from the PR (the workflow and this script stay default-branch); entries whose id the - default branch does not know are marked `new`. Raises when the PR's copy is unreadable. +def head_manifest(default: dict, target: dict, files: list[str] | None) -> dict: + """The manifest to run from: the one at the F´ revision under test (PR head, merge-queue commit or + manual ref), so a PR that adds or edits an entry exercises it on its own checks. It is read as + data through the contents API of this repo (PR head commits are reachable here, fork or not); the + workflow and this script stay default-branch. When the PR edits manifest.json (`files`), entries + whose id the default branch does not have are marked `new`; a stale branch that merely predates a + removal gets no such mark. Raises when the file is missing, malformed or has duplicate ids. """ - if not target["pr"] or not files or MANIFEST_PATH not in files: - return manifest - blob = own.get( - f"/repos/{target['head_repo']}/contents/{MANIFEST_PATH}?ref={target['head_sha']}" - ) + try: + blob = own.get( + f"/repos/{REPO}/contents/{MANIFEST_PATH}?ref={target['head_sha']}" + ) + except ApiError as e: + if e.status == 404: + raise RuntimeError( + f"no {MANIFEST_PATH} at {fprime_rev(target)} (predates external CI?)" + ) from e + raise head = json.loads(base64.b64decode(blob["content"])) - known = {entry["id"] for entry in manifest["repos"]} + ids = [entry["id"] for entry in head["repos"]] + if len(ids) != len(set(ids)): + raise RuntimeError(f"duplicate ids in {MANIFEST_PATH} at {fprime_rev(target)}") + known = {entry["id"] for entry in default["repos"]} + edited = files is None or MANIFEST_PATH in files for entry in head["repos"]: - entry["new"] = entry["id"] not in known - log(f"using the PR's manifest.json ({target['head_sha'][:7]})") + entry["new"] = edited and entry["id"] not in known + new = [entry["id"] for entry in head["repos"] if entry["new"]] + log( + f"manifest.json from {fprime_rev(target)}: {ids}" + + (f", new: {new}" if new else "") + ) return head @@ -475,7 +490,7 @@ def dispatch_one(manifest: dict, entry: dict, target: dict, state: dict) -> None ) as e: # noqa: BLE001 - one repo's failure must not stop the fan-out hint = ( " (no such workflow, or the repository is not in the External CI App installation)" - if isinstance(e, ApiError) and e.status == 404 + if isinstance(e, ApiError) and e.status in (403, 404) else "" ) s["error"], s["remote_conclusion"] = ( @@ -607,13 +622,12 @@ def failure_details(s: dict) -> str: def check_conclusion(s: dict) -> str: - """Map the remote result to the Check Run conclusion: success, neutral (any failure on a - `failure_policy: notify` repo), cancelled, timed_out or failure.""" + """Map the remote result to the Check Run conclusion: success, cancelled, timed_out or failure. + `failure_policy` does not soften it (GitHub folds neutral checks away; a non-blocking failure + must still show red): it only decides whether this job fails, see run().""" rc = s["remote_conclusion"] if rc == "success": return "success" - if s["failure_policy"] == "notify": - return "neutral" # visible, but does not block the PR if rc == "cancelled": return "cancelled" if rc in ("timed_out", "queue_timeout"): @@ -640,7 +654,7 @@ def finalize_one(target: dict, s: dict) -> None: if s["failure_policy"] == "notify" and s["remote_conclusion"] != "success": lines += [ "", - f"This repo is {code('failure_policy: notify')}: the failure does not block the PR.", + f"This repo is {code('failure_policy: notify')}: informational, the failure does not block merging.", ] lines += ["", f"Orchestrator run: {ORCHESTRATOR_URL}"] output = { @@ -737,11 +751,12 @@ def write_summary(target: dict, state: dict) -> None: # ---------------------------------------------------------------- modes def run() -> None: - """The `run` mode: resolve target, load manifest, fork preflight, select repos, dispatch, poll, - close checks, write summary. Exits 1 when a `failure_policy: block` repo did not succeed or the - orchestrator itself failed on any repo.""" + """The `run` mode: resolve target, fork preflight, load the target's manifest, select repos, + dispatch, poll, close checks, write summary. Exits 1 when a `failure_policy: block` repo did not + succeed or the orchestrator itself failed on any repo.""" target = resolve_target() log(f"target: {json.dumps(target)}") + # The default branch's copy: trusted, used for the preflight below and as the `new` baseline. with open(MANIFEST_PATH) as f: manifest = json.load(f) # Fork preflight: a fork of F´ that copies this workflow must not dispatch into the ext repos. @@ -753,7 +768,7 @@ def run() -> None: ) return files = changed_files(target) - manifest = pr_manifest(manifest, target, files) + manifest = head_manifest(manifest, target, files) repos = select_repos(manifest, target) if ATTEMPT > 1: # re-run: only the repos whose check is not green yet repos = [e for e in repos if not check_succeeded(manifest, e, target)] @@ -821,7 +836,7 @@ def finalize() -> None: except ApiError: pass if cancelled: - conclusion = "neutral" if s["failure_policy"] == "notify" else "cancelled" + conclusion = "cancelled" title, why = ( "Cancelled by orchestrator", "The orchestrator run was cancelled", @@ -835,7 +850,7 @@ def finalize() -> None: ) why = "Closed by the finalize step after the check update failed" else: - conclusion = "neutral" if s["failure_policy"] == "notify" else "failure" + conclusion = "failure" title = "Orchestrator error" why = f"The orchestrator run ended before this repo was reported ({code(s.get('error') or 'orchestrator error')})" try: