Repository navigation
ci: dispatch external CI to the external repositories (led-blinker pilot) - #297
devin-ai-integration[bot] wants to merge 22 commits into
Conversation
…rator) Adds ext-ci-trigger.yml (no-op pull_request gate), ext-ci.yml (workflow_run orchestrator: dispatch, poll, one Check Run per external repo, failed-only re-run, notify/block policy) and .github/ext-ci-manifest.json onboarding led-blinker, zephyr-reference and generic-hub-reference in the notify tier. Existing ext-*.yml workflows are untouched and keep running alongside.
…o dispatch errors are visible
|
I'll fix CI failures and address comments from users with write access that start with 'Devin'.
Prompt hidden (unlisted session) |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.
Devin Review found 5 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| pr_number: | ||
| description: "PR to test and report on (takes precedence over fprime_ref)" | ||
| type: string |
There was a problem hiding this comment.
🟨 Manual dispatch accepts unapproved PR heads
A manual pr_number runs privileged external CI for a fork PR without the trigger workflow's approval gate. Users who can start this workflow can send unapproved fork code into external CI.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
By design: workflow_dispatch is only available to users with write access, which is the same set of people who can press "Approve and run" on a fork PR, so a manual pr_number run is an explicit approval by a maintainer. The dispatched code never runs with secrets here either (contents: read, no secrets on the external side).
Same behaviour and workflows; folds in review findings: no automatic retry of the non-idempotent dispatch call (a lost response falls back to dispatch_id correlation), a failed check-status update is not treated as a failed dispatch, and a check whose completion update failed is closed by the finalize step with the remote result.
…nifest source, compare 404, concurrency key) - correlate/poll_all tolerate carried-forward entries (no remote_* keys) on a re-run - a manual run with fprime_ref reads the manifest and ext ref from the branch the run was started from, not from the ref under test (any PR SHA is readable via /contents?ref=) - a release push whose before SHA was rewritten runs everything instead of aborting - concurrency group includes the event name so a PR branch named release/* cannot cancel the push run
Drop owner notifications (a notify repo is just a neutral check; no pull-requests/issues write needed any more), failed-only re-run carry-forward (a re-run re-dispatches every repo), the dispatch_id correlation fallback (the dispatch API returns the run id), manifest lookup from the base branch and ETag polling.
There was a problem hiding this comment.
Note
Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.
Devin Review found 1 new potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| with open(MANIFEST_PATH) as f: | ||
| manifest = json.load(f) |
There was a problem hiding this comment.
🔴 Release PRs use the wrong manifest
For a release PR, run reads the default-branch manifest instead of the PR base's manifest. The default-branch checkout can select the wrong external repos or skip required checks.
Learn more
For a workflow_run event, the orchestrator checks out the default branch, even when the completed trigger belongs to a PR targeting a release branch. resolve_target stores that PR's base ref, but opening the local manifest discards it. The manifest controls both external repository selection and ignored paths, so a release PR can receive checks from the wrong configuration.
Example: Suppose release/2 adds a second external repository to its manifest while devel lists only led-blinker. A PR targeting release/2 gets only led-blinker checked, despite the release branch requiring both.
Recommended fix: Fetch and parse the manifest at target['base_ref'] using the trusted repository API. Preserve the existing missing-file fallback without treating other API errors as missing manifests.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Known trade-off of the pilot simplification in 6052882 (reading the manifest from the PR base branch was dropped on purpose to keep the script small). It only affects PRs that target release/** while the release branch's manifest differs from devel's; nightly/push runs on a release branch use that branch's own file. If a release branch ever needs a different repo list, the base-branch lookup comes back (~15 lines).
There was a problem hiding this comment.
[Design] Concur — also in scope for design fit: the cited plan (§5.1) compensates for the default-branch orchestrator by reading the manifest from the PR base branch; the pilot dropped that and neither the PR description nor the manifest records the release/** trade-off; severity from my scope: could fix.
| for i in range(5): | ||
| req = urllib.request.Request(API + url, data=data, method=method, headers=headers) | ||
| try: | ||
| with urllib.request.urlopen(req, timeout=60) as res: | ||
| status, hdrs, text = res.status, res.headers, res.read().decode() | ||
| except urllib.error.HTTPError as e: | ||
| status, hdrs, text = e.code, e.headers, e.read().decode(errors="replace") | ||
| except (urllib.error.URLError, OSError) as e: | ||
| if not idempotent or i == 4: | ||
| raise ApiError(f"{self.label}: {method} {url} -> network error {e}") | ||
| time.sleep(2 * 2**i) | ||
| continue | ||
| limited = status == 429 or ( | ||
| status == 403 and (hdrs.get("x-ratelimit-remaining") == "0" or re.search("rate limit", text, re.I)) | ||
| ) | ||
| if (limited or (status >= 500 and idempotent)) and i < 4: |
There was a problem hiding this comment.
[Maintainability] suggestion maint-structural-hazard: the retry bound is encoded three times (range(5), i == 4, i < 4).
The last-attempt test is what keeps raise AssertionError("unreachable") at the end of the loop unreachable. Bumping range(5) alone compiles and runs, but the final network error on a retryable call then takes continue out of the loop and lands on that assert; one named bound keeps the three sites in step.
| for i in range(5): | |
| req = urllib.request.Request(API + url, data=data, method=method, headers=headers) | |
| try: | |
| with urllib.request.urlopen(req, timeout=60) as res: | |
| status, hdrs, text = res.status, res.headers, res.read().decode() | |
| except urllib.error.HTTPError as e: | |
| status, hdrs, text = e.code, e.headers, e.read().decode(errors="replace") | |
| except (urllib.error.URLError, OSError) as e: | |
| if not idempotent or i == 4: | |
| raise ApiError(f"{self.label}: {method} {url} -> network error {e}") | |
| time.sleep(2 * 2**i) | |
| continue | |
| limited = status == 429 or ( | |
| status == 403 and (hdrs.get("x-ratelimit-remaining") == "0" or re.search("rate limit", text, re.I)) | |
| ) | |
| if (limited or (status >= 500 and idempotent)) and i < 4: | |
| attempts = 5 | |
| for i in range(attempts): | |
| last = i == attempts - 1 | |
| req = urllib.request.Request(API + url, data=data, method=method, headers=headers) | |
| try: | |
| with urllib.request.urlopen(req, timeout=60) as res: | |
| status, hdrs, text = res.status, res.headers, res.read().decode() | |
| except urllib.error.HTTPError as e: | |
| status, hdrs, text = e.code, e.headers, e.read().decode(errors="replace") | |
| except (urllib.error.URLError, OSError) as e: | |
| if not idempotent or last: | |
| raise ApiError(f"{self.label}: {method} {url} -> network error {e}") | |
| time.sleep(2 * 2**i) | |
| continue | |
| limited = status == 429 or ( | |
| status == 403 and (hdrs.get("x-ratelimit-remaining") == "0" or re.search("rate limit", text, re.I)) | |
| ) | |
| if (limited or (status >= 500 and idempotent)) and not last: |
There was a problem hiding this comment.
Left as is: three sites in one 30-line function; will name the bound if the client grows.
| state["repos"][entry["id"]] = { | ||
| "id": entry["id"], "repository": entry["repository"], "failure_policy": entry.get("failure_policy", "block"), | ||
| "check_run_id": check["id"], "remote_conclusion": "skipped", "conclusion": "skipped", | ||
| } |
There was a problem hiding this comment.
[Maintainability] could fix maint-code-duplication: a second, narrower shape of the per-repo state record.
dispatch_one builds the record with 13 keys; this one has 6, so downstream code has to guess which keys exist: finalize() reads s.get("remote_run_id") / s.get("ext_ref") but s["failure_policy"] / s["id"], and write_summary mixes both styles on the same record. A new consumer that indexes s["ext_ref"] or s["error"] passes on the dispatch path and raises KeyError on the skipped path. One new_repo_state(entry) helper (full key set, remote_conclusion / conclusion overridden here) gives both paths one shape and lets every reader use plain indexing.
There was a problem hiding this comment.
Left as is for the pilot: the maintainer preferred not to add a constructor; the skip path's narrower record is read with .get() deliberately.
| rc = s["remote_conclusion"] | ||
| if rc == "success": | ||
| return "success" | ||
| if s["failure_policy"] == "notify": |
There was a problem hiding this comment.
[Maintainability] could fix maint-code-duplication: the failure_policy == "notify" rule is re-derived at four sites.
Here, in finalize_one (l.391), in run()'s blocking filter (l.469) and in finalize() (l.502), each with its own mapping to neutral / non-blocking; the "block" default is spelled out twice more (l.269, l.414). Adding a third policy value, or renaming this one, means finding all six by grep; one is_blocking(s) predicate (or routing l.469 and l.502 through check_conclusion) makes the manifest's policy vocabulary live in one place.
There was a problem hiding this comment.
Left as is for the pilot: the rule now lives at three sites (check_conclusion, the blocking filter in run, finalize), each commented; a third policy value would be the moment to centralise it.
|
|
||
|
|
||
| def code(s): | ||
| # Remote-controlled text (job/step names, branch names) is only ever rendered inside code spans. |
There was a problem hiding this comment.
[Maintainability] could fix maint-misleading-comment: "only ever rendered inside code spans" is broader than the code.
ext_ref (a branch name, named in this comment) is interpolated raw into the Check Run title at l.300, l.394 and l.497. Titles are plain text, so there is no rendering hazard today, but a reader taking this comment as the invariant will not look for the exemption; saying "in Markdown bodies" (or routing titles through code() too) keeps the comment true.
cc @LeStarch @thomas-bc — low-confidence finding, please confirm.
There was a problem hiding this comment.
Fixed in 219317e: the helper now carries a docstring stating the actual rule, and the one raw use of head_branch (summary title) goes through it.
| "workflow": "ci.yml", | ||
| "default_ref": "devel", | ||
| "timeout_minutes": 60, | ||
| "failure_policy": "notify", |
There was a problem hiding this comment.
[Documentation] could fix CONTRIBUTING.md § "Automated Checks on Reference Repositories" does not describe the new ext-ci / <id> check contributors will now see on their PRs.
(Anchored here; the finding is on CONTRIBUTING.md, which this PR does not touch.) That section still only describes the in-repo ext-*.yml builds. The new check runs the external repository's own ci.yml against the PR head, appears only after a maintainer approves the fork workflow run, and with failure_policy: notify a failure shows as a neutral (non-blocking) check. Branch selection (pr-<N>, then the base branch, then devel) is unchanged, so the existing sentence stays true; the section is incomplete rather than wrong, hence not must fix. Proposed addition after the pr-<PR_NUMBER> paragraph:
Some reference repositories are checked through their own CI instead: F´ dispatches the repository's `ci.yml` against your PR head and reports the result as a check named `ext-ci / <repository id>` (see `.github/ext-ci-manifest.json`). For pull requests from forks this check appears once a maintainer has approved the workflow run. Repositories marked `failure_policy: notify` in the manifest report failures as a neutral, non-blocking check.There was a problem hiding this comment.
Deferred: CONTRIBUTING.md will be updated when ext-build-led-blinker.yml is retired in favour of this check, so contributors read one description, not two.
| For every repo in .github/ext-ci-manifest.json: create a Check Run on the F´ commit under test, | ||
| dispatch the repo's own ci.yml with that commit, poll the remote run, close the Check Run with | ||
| its result. A re-run of the orchestrator re-dispatches every repo. | ||
|
|
||
| Two credentials, never mixed: | ||
| GITHUB_TOKEN this repo only (checks: write) - reports results | ||
| DISPATCH_TOKEN external repos only (actions: write; contents: read too if an external repo | ||
| is private, for the branch lookup) - starts remote runs |
There was a problem hiding this comment.
[Documentation] suggestion The manifest schema and the contract an external ci.yml must satisfy are not documented anywhere in the repository.
This header is the only place a maintainer onboarding the next fprime-community repo can look: .github/ext-ci-manifest.json is JSON (no comments) and nothing under docs/ or CONTRIBUTING.md names upstream_repository, check_prefix, paths_ignore, default_ref, timeout_minutes, failure_policy, on_pr, the EXT_CI_DISPATCH_TOKEN secret, or the four workflow_dispatch inputs the external workflow has to accept (fprime_repository, fprime_ref, fprime_pr_number, dispatch_id). Field defaults below are taken from dispatch_one / select_repos / resolve_ext_ref / check_name.
| For every repo in .github/ext-ci-manifest.json: create a Check Run on the F´ commit under test, | |
| dispatch the repo's own ci.yml with that commit, poll the remote run, close the Check Run with | |
| its result. A re-run of the orchestrator re-dispatches every repo. | |
| Two credentials, never mixed: | |
| GITHUB_TOKEN this repo only (checks: write) - reports results | |
| DISPATCH_TOKEN external repos only (actions: write; contents: read too if an external repo | |
| is private, for the branch lookup) - starts remote runs | |
| For every repo in .github/ext-ci-manifest.json: create a Check Run on the F´ commit under test, | |
| dispatch the repo's own ci.yml with that commit, poll the remote run, close the Check Run with | |
| its result. A re-run of the orchestrator re-dispatches every repo. | |
| Manifest (.github/ext-ci-manifest.json): | |
| upstream_repository only this repo dispatches on push/schedule/workflow_run (forks: workflow_dispatch only) | |
| check_prefix Check Run name is "<check_prefix> / <id>" (default "ext-ci") | |
| paths_ignore fnmatch globs; a PR/push touching only these gets skipped Check Runs | |
| repos[] id, repository (owner/name), workflow (file name under .github/workflows/), | |
| default_ref (default "main"; tried after pr-<N> and the F´ base branch), | |
| timeout_minutes (default 60, counted from remote start), failure_policy | |
| ("block" default, or "notify" = neutral check on failure), on_pr (default true) | |
| External workflow contract: `workflow` must accept workflow_dispatch inputs fprime_repository, | |
| fprime_ref, fprime_pr_number (may be empty) and dispatch_id, and check out that F´ revision itself. | |
| Two credentials, never mixed: | |
| GITHUB_TOKEN this repo only (checks: write) - reports results | |
| DISPATCH_TOKEN repository secret EXT_CI_DISPATCH_TOKEN, external repos only (actions: write; | |
| contents: read too if an external repo is private, for the branch lookup) | |
| - starts remote runs |
cc @LeStarch @thomas-bc — low-confidence finding, please confirm (the right home for this may be CONTRIBUTING.md or a docs/ page rather than the script header).
There was a problem hiding this comment.
Fixed in d2045d3: the script header now lists every manifest field and the contract an external ci.yml must satisfy.
| env: | ||
| GITHUB_TOKEN: ${{ github.token }} | ||
| DISPATCH_TOKEN: ${{ secrets.EXT_CI_DISPATCH_TOKEN }} | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - name: "Dispatch, poll and report" | ||
| env: | ||
| INPUT_REPOS: ${{ inputs.repos }} | ||
| INPUT_PR_NUMBER: ${{ inputs.pr_number }} | ||
| INPUT_FPRIME_REF: ${{ inputs.fprime_ref }} | ||
| POLL_INTERVAL_SECONDS: 60 | ||
| run: python3 .github/scripts/ext-ci-orchestrator.py run | ||
| - name: "Finalize (close open checks, cancel orphaned remote runs)" | ||
| if: always() | ||
| env: | ||
| JOB_STATUS: ${{ job.status }} | ||
| run: python3 .github/scripts/ext-ci-orchestrator.py finalize |
There was a problem hiding this comment.
[Supply Chain] suggestion DISPATCH_TOKEN is a job-level env var, so it is also handed to the actions/checkout and actions/upload-artifact steps that never need it.
Only the two ext-ci-orchestrator.py steps use the external-repo credential. Step-scoping it keeps the secret out of the process environment of every third-party action in the job (least privilege for the one credential in this design whose leak gives actions: write on the external repos).
| env: | |
| GITHUB_TOKEN: ${{ github.token }} | |
| DISPATCH_TOKEN: ${{ secrets.EXT_CI_DISPATCH_TOKEN }} | |
| steps: | |
| - uses: actions/checkout@v4 | |
| - name: "Dispatch, poll and report" | |
| env: | |
| INPUT_REPOS: ${{ inputs.repos }} | |
| INPUT_PR_NUMBER: ${{ inputs.pr_number }} | |
| INPUT_FPRIME_REF: ${{ inputs.fprime_ref }} | |
| POLL_INTERVAL_SECONDS: 60 | |
| run: python3 .github/scripts/ext-ci-orchestrator.py run | |
| - name: "Finalize (close open checks, cancel orphaned remote runs)" | |
| if: always() | |
| env: | |
| JOB_STATUS: ${{ job.status }} | |
| run: python3 .github/scripts/ext-ci-orchestrator.py finalize | |
| env: | |
| GITHUB_TOKEN: ${{ github.token }} | |
| steps: | |
| - uses: actions/checkout@v4 | |
| - name: "Dispatch, poll and report" | |
| env: | |
| DISPATCH_TOKEN: ${{ secrets.EXT_CI_DISPATCH_TOKEN }} | |
| INPUT_REPOS: ${{ inputs.repos }} | |
| INPUT_PR_NUMBER: ${{ inputs.pr_number }} | |
| INPUT_FPRIME_REF: ${{ inputs.fprime_ref }} | |
| POLL_INTERVAL_SECONDS: 60 | |
| run: python3 .github/scripts/ext-ci-orchestrator.py run | |
| - name: "Finalize (close open checks, cancel orphaned remote runs)" | |
| if: always() | |
| env: | |
| DISPATCH_TOKEN: ${{ secrets.EXT_CI_DISPATCH_TOKEN }} | |
| JOB_STATUS: ${{ job.status }} | |
| run: python3 .github/scripts/ext-ci-orchestrator.py finalize |
There was a problem hiding this comment.
[Security] Concur — also in scope for secret handling (general-vulnerability/secret-handling): only the two script steps need the PAT that can start, cancel, re-run and approve runs on every external repo; the step-scoped env: above is the right shape.
There was a problem hiding this comment.
Fixed in 219317e: DISPATCH_TOKEN is now set only on the two orchestrator steps (run, finalize); checkout and upload-artifact no longer see it.
| link = f"[run]({s['remote_run_url']})" if s.get("remote_run_url") else (code(s["error"]) if s.get("error") else "") | ||
| rows.append(f"| {code(s['id'])} | {s['repository']} | {code(s.get('ext_ref') or '-')} | {s['failure_policy']} " | ||
| f"| {s['remote_conclusion']} | {s['conclusion']} | {link} |") | ||
| where = f" (PR #{target['pr']})" if target["pr"] else f" ({target['head_branch']})" |
There was a problem hiding this comment.
[Supply Chain] suggestion PR-controlled head_branch is written to $GITHUB_STEP_SUMMARY outside a code span.
target['head_branch'] comes from the fork PR (workflow_run.head_branch) and here lands raw in the rendered job summary, which contradicts the code() contract at the top of the file ("remote-controlled text ... only ever rendered inside code spans"). Git refname rules limit what can be smuggled (no spaces/newlines), but autolinks and inline HTML fragments still render; wrapping it closes the gap at no cost.
| where = f" (PR #{target['pr']})" if target["pr"] else f" ({target['head_branch']})" | |
| where = f" (PR #{target['pr']})" if target["pr"] else f" ({code(target['head_branch'])})" |
There was a problem hiding this comment.
[Security] Concur — also in scope for CI test-runtime policy (general-vulnerability/markdown-injection): the fork-chosen workflow_run.head_branch is the only remote string that bypasses code(), and the one-line wrap above closes it.
There was a problem hiding this comment.
Fixed in 219317e: head_branch goes through code() in the summary title.
| 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(r["id"] == i for r in repos)] | ||
| if unknown: | ||
| raise RuntimeError("unknown manifest ids: " + ", ".join(unknown)) | ||
| repos = [r for r in repos if r["id"] in ids] | ||
| elif target["pr"]: | ||
| repos = [r for r in repos if r.get("on_pr", True)] | ||
| return repos |
There was a problem hiding this comment.
[Supply Chain] suggestion on_pr: false is skipped when a manual run names repos explicitly, so a PR head (including a fork's) can be dispatched to a repo that opted out of PR heads.
on_pr is the only manifest knob that can keep untrusted F´ PR code off an external repo whose ci.yml runs on persistent self-hosted runners (apple-ci, Zephyr boards, RPi); workflow_dispatch with repos=<id> + pr_number bypasses it. Honoring the flag whenever target["pr"] is set keeps that guardrail intact on manual runs too (nightly/release runs are unaffected).
| 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(r["id"] == i for r in repos)] | |
| if unknown: | |
| raise RuntimeError("unknown manifest ids: " + ", ".join(unknown)) | |
| repos = [r for r in repos if r["id"] in ids] | |
| elif target["pr"]: | |
| repos = [r for r in repos if r.get("on_pr", True)] | |
| return repos | |
| 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(r["id"] == i for r in repos)] | |
| if unknown: | |
| raise RuntimeError("unknown manifest ids: " + ", ".join(unknown)) | |
| repos = [r for r in repos if r["id"] in ids] | |
| if target["pr"]: | |
| # on_pr: false opts a repo out of PR heads (e.g. CI on persistent self-hosted runners); honored on manual runs too | |
| repos = [r for r in repos if r.get("on_pr", True)] | |
| return repos |
cc @LeStarch @thomas-bc — low-confidence finding, please confirm.
There was a problem hiding this comment.
[Design] Concur — also in scope for design fit: on_pr: false is the manifest's only PR-origin guard, so it must be a hard rule rather than a default filter before any self-hosted-runner repo is added; pair it on the external side with if: inputs.fprime_pr_number == '' on self-hosted jobs.
There was a problem hiding this comment.
[Security] Concur — also in scope for CI test-runtime policy (runner-safety): on_pr additionally defaults to true at l.244 (r.get("on_pr", True)), so an entry that omits it receives fork-PR code on the next push; make it opt-in (r.get("on_pr") is True) and add a manifest runner-class field (e.g. runners: github-hosted) that the orchestrator checks before dispatching any PR head, because on persistent self-hosted hardware (apple-ci, Zephyr boards, RPi, stm32) no token scope or cache mode contains untrusted F´ code.
There was a problem hiding this comment.
Fixed in 219317e: the on_pr filter now applies whenever a PR is the target, manual runs included.
| timeout-minutes: 180 | ||
| env: | ||
| GITHUB_TOKEN: ${{ github.token }} | ||
| DISPATCH_TOKEN: ${{ secrets.EXT_CI_DISPATCH_TOKEN }} |
There was a problem hiding this comment.
[Design] must fix Human design adjudication required. A new privileged CI mechanism (new secret, new check contract, new manifest) lands with two design-doc decisions still open.
The plan this PR cites leaves §7 decisions 1 and 2 unrecorded: (1) the dispatch credential — §5.2 recommends a dedicated GitHub App over a PAT (expiry, personal attribution, per-run minting) and this PR wires a repository-secret PAT without saying why; (2) self-hosted runners (§5.3) — the manifest has no trust-class field, so nothing but on_pr and reviewer memory keeps an apple-ci / Zephyr / Raspberry Pi repo from receiving fork-PR code once it is added. Both decide what the manifest and the external ci.yml contract must guarantee before a second repo is added and before the ext-*.yml retirements start; a design-owner sign-off (or a pointer to where these were decided) belongs in the PR description now rather than after rollout.
cc @LeStarch @thomas-bc — design needs human adjudication before deeper review.
There was a problem hiding this comment.
Decision: for the pilot, EXT_CI_DISPATCH_TOKEN is a fine-grained personal access token scoped to the fprime-community/fprime-workshop-led-blinker repository only (Actions read/write; no permission on nasa/fprime). A GitHub App with short-lived installation tokens replaces it before any further repository is added to the manifest.
| s["ext_ref"] = resolve_ext_ref(entry, target) | ||
| r = ext.post(f"/repos/{entry['repository']}/actions/workflows/{entry['workflow']}/dispatches", { | ||
| "ref": s["ext_ref"], | ||
| "inputs": {"fprime_repository": target["head_repo"], "fprime_ref": target["head_sha"], |
There was a problem hiding this comment.
[Design] could fix External CI now tests the PR head alone; the ext-build-led-blinker.yml it replaces built the PR merge commit.
reusable-project-builder.yml overlays F´ with a default actions/checkout, which on pull_request is refs/pull/<N>/merge; here the external side checks out head_repo@head_sha, so a PR branched from an old devel passes ext-ci while its merge with devel would not (only the merge-queue path still exercises a merge). A defensible trade-off — the merge ref is absent for conflicting PRs and lags force-pushes — but the PR description should state it, since this check is meant to become required.
There was a problem hiding this comment.
Accepted trade-off: the external workflow checks out an exact SHA, and the PR head is what the Check Run is attached to; the merge-queue run tests the merge before it lands. Recorded in the PR description.
| blocking = [s["id"] for s in state["repos"].values() | ||
| if s["failure_policy"] != "notify" and (s["conclusion"] or check_conclusion(s)) not in ("success", "skipped")] |
There was a problem hiding this comment.
[Operational] must fix ops-failure-path: notify also silences orchestrator-side faults and non-PR runs, so the job exits 0 and nobody is told.
Scenario: EXT_CI_DISPATCH_TOKEN expires (fine-grained PATs: ≤ 1 year) or the nightly led-blinker build breaks after ext-build-led-blinker.yml is retired. check_conclusion maps dispatch_failed / queue_timeout / error and every nightly failure to neutral, this filter excludes them, the "External CI" job is green and the only trace is a grey check on a devel commit nobody opens — external CI is silently off for every PR until someone notices. Smallest remedy: keep notify non-blocking for PR checks but fail the job when the fault is ours (error set) or there is no PR to spare (nightly / release/** push), so the scheduled-failure notification fires as it did for ext-*.yml. Operational judgment call on the notify semantics.
| blocking = [s["id"] for s in state["repos"].values() | |
| if s["failure_policy"] != "notify" and (s["conclusion"] or check_conclusion(s)) not in ("success", "skipped")] | |
| blocking = [s["id"] for s in state["repos"].values() | |
| if (s["failure_policy"] != "notify" or s.get("error") or not target["pr"]) | |
| and (s["conclusion"] or check_conclusion(s)) not in ("success", "skipped")] |
cc @LeStarch @thomas-bc — low-confidence finding, please confirm.
There was a problem hiding this comment.
Fixed in 219317e: notify only softens a remote build failure; orchestrator-side faults (dispatch_failed, queue_timeout, error) fail the job for every repo. The nightly and release triggers were removed in the same push (external repos run their own nightly CI), so the non-PR case is gone.
| r = ext.post(f"/repos/{entry['repository']}/actions/workflows/{entry['workflow']}/dispatches", { | ||
| "ref": s["ext_ref"], | ||
| "inputs": {"fprime_repository": target["head_repo"], "fprime_ref": target["head_sha"], | ||
| "fprime_pr_number": str(target["pr"] or ""), "dispatch_id": f"{RUN_ID}-{ATTEMPT}-{entry['id']}"}, | ||
| "return_run_details": True, | ||
| }, idempotent=False) |
There was a problem hiding this comment.
[Operational] could fix ops-failure-path: a dispatch whose response is lost is reported as dispatch_failed while the remote run proceeds untracked.
Scenario: the POST times out or gets a 502 after GitHub created the run (idempotent=False correctly refuses to retry). Consequence: the check closes as dispatch_failed (neutral/failure), the external run burns up to timeout_minutes of its runner with nobody polling or cancelling it, and its real result is lost; the correlate()-by-dispatch_id fallback that closed this gap in a18ed2c was dropped in the simplification. Bounded (one orphan run, one re-run) — smallest remedy: on a network/5xx error here, list workflow_dispatch runs created since dispatched_at and adopt the one whose display_title contains dispatch_id before declaring failure.
There was a problem hiding this comment.
[Correctness] Concur — also in scope for correctness (correctness-resource-leak): the same orphan arises when the POST returns without workflow_run_id (l.294) or SIGINT lands during the return_run_details wait on cancellation, and since remote_conclusion = "dispatch_failed" makes both poll_all() and finalize() (l.487) skip the entry the run is never cancelled — contrary to the finalize() docstring, and the .mjs-thread reply that relies on correlate() no longer holds in 6052882; severity from my scope: must fix.
There was a problem hiding this comment.
[Summary] Promoted to must fix — state left inconsistent on an error path: a lost dispatch response records dispatch_failed while the remote run proceeds untracked and is never cancelled.
The summary counts this finding as must fix; the original tag above stands as posted.
There was a problem hiding this comment.
Accepted as is: the case needs GitHub to accept the dispatch while the HTTP response is lost; the result is a visibly failed check ("dispatch failed", never a wrong success) and one untracked remote run that finishes on its own. Re-running the orchestrator for that PR repairs it. Not worth the extra code for the pilot.
| except ApiError as e: | ||
| # A single bad poll must not take the whole fan-out down; give up on this repo only | ||
| # when the credential is gone or the run vanished. | ||
| log(f"::warning::[{s['id']}] poll: {e}") | ||
| if e.status in (401, 404): | ||
| s["remote_conclusion"], s["error"] = "error", f"poll failed: {e}" |
There was a problem hiding this comment.
[Operational] could fix ops-timing-resource: both timeouts live inside the try, so while polls fail the repo never times out.
Scenario: a sustained API outage or a PAT that lost actions: read after dispatch (403, not 401/404). Each poll raises, this branch only warns, and neither the timeout_minutes nor the 2 × timeout queue check is evaluated — the loop holds the runner for the full 180-min job timeout, after which finalize() closes the check as cancelled (red even under notify). Smallest remedy: bound consecutive poll failures (15 min at POLL_S = 60 → 15 strikes) and give up with error like the 401/404 case.
| except ApiError as e: | |
| # A single bad poll must not take the whole fan-out down; give up on this repo only | |
| # when the credential is gone or the run vanished. | |
| log(f"::warning::[{s['id']}] poll: {e}") | |
| if e.status in (401, 404): | |
| s["remote_conclusion"], s["error"] = "error", f"poll failed: {e}" | |
| except ApiError as e: | |
| # A single bad poll must not take the whole fan-out down; give up on this repo only | |
| # when the credential is gone, the run vanished, or polling has failed for 15 min straight. | |
| log(f"::warning::[{s['id']}] poll: {e}") | |
| s["poll_failures"] = s.get("poll_failures", 0) + 1 | |
| if e.status in (401, 404) or s["poll_failures"] * POLL_S >= 15 * 60: | |
| s["remote_conclusion"], s["error"] = "error", f"poll failed: {e}" | |
| else: | |
| s["poll_failures"] = 0 |
There was a problem hiding this comment.
[Correctness] Concur — also in scope for correctness (correctness-nontermination): the per-repo timeout_minutes contract is simply unenforced on the error path, and bounding it on the clock already kept (now() - parse_ts(s["dispatched_at"]) > 3 * timedelta(minutes=s["timeout_minutes"])) needs no new state; severity from my scope: suggestion.
There was a problem hiding this comment.
Left as is for the pilot: a sustained poll outage is bounded by the job's timeout-minutes, after which finalize closes the check (neutral for notify repos since d2045d3). Will add a poll-failure budget if it ever happens.
| conclusion, title = check_conclusion(s), f"{s['remote_conclusion']} ({s['repository']} @ {s.get('ext_ref') or '-'})" | ||
| why = "Closed by the finalize step after the check update failed" | ||
| elif cancelled: | ||
| conclusion, title, why = "cancelled", "Cancelled by orchestrator", "The orchestrator run was cancelled" |
There was a problem hiding this comment.
[Operational] could fix ops-failure-path: the cancellation path ignores failure_policy, so a notify repo can still block a PR.
Scenario: the remote run queues for up to 2 × timeout_minutes (120 min) and then runs for up to 60 min — exactly the job's timeout-minutes: 180, with checkout, dispatch and the 60-s poll granularity on top — so GitHub kills the job a minute or two before poll_all would have said timed_out, and this branch closes the check as cancelled, which branch protection counts as a failure. Rare but legal; smallest remedy: honor notify here (and/or give the job timeout margin over 3 × timeout_minutes).
| conclusion, title, why = "cancelled", "Cancelled by orchestrator", "The orchestrator run was cancelled" | |
| conclusion = "neutral" if s["failure_policy"] == "notify" else "cancelled" | |
| title, why = "Cancelled by orchestrator", "The orchestrator run was cancelled" |
There was a problem hiding this comment.
Fixed in d2045d3: a cancelled notify repo is closed as neutral.
| def resolve_ext_ref(entry, target): | ||
| """pr-<N> branch on the ext repo, else the F´ base branch name, else the manifest default.""" | ||
| for c in (f"pr-{target['pr']}" if target["pr"] else None, target["base_ref"]): | ||
| if c and ext.exists(f"/repos/{entry['repository']}/branches/{q(c)}"): | ||
| return c | ||
| return entry.get("default_ref", "main") |
There was a problem hiding this comment.
[Security] must fix PR builds dispatched onto the external repo's default branch give untrusted F´ code default-branch Actions-cache write scope (ci-test-runtime-policy-violation/cache-tampering).
Fork-PR runs land on devel unless a pr-N branch exists, so the dispatched job's GITHUB_REF is led-blinker's default branch and whatever it saves to the Actions cache is restored by every later run there.
ccache: false / cache-mode: read (fprime-actions#22) only stop setup's own saves; code the PR controls (its CMake, lib/fprime/requirements.txt installs) runs as the same uid as Runner.Worker, can take ACTIONS_RUNTIME_TOKEN from it and save any key in that scope.
Later dispatched runs still restore pip and led-blinker's own devel runs restore ccache+pip, so one malicious F´ PR persists into led-blinker CI and can shape ext-ci verdicts of unrelated PRs; today's in-repo pull_request builds are cache-scoped to their PR, so this widens the exposure into another org's repo.
Fix: dispatch PR runs to a dedicated non-default ref per repo (manifest pr_ref, e.g. ext-ci; best-effort below) and require dispatched external workflows to neither save nor restore the Actions cache; self-hosted-runner repos need more (see the on_pr thread).
| def resolve_ext_ref(entry, target): | |
| """pr-<N> branch on the ext repo, else the F´ base branch name, else the manifest default.""" | |
| for c in (f"pr-{target['pr']}" if target["pr"] else None, target["base_ref"]): | |
| if c and ext.exists(f"/repos/{entry['repository']}/branches/{q(c)}"): | |
| return c | |
| return entry.get("default_ref", "main") | |
| def resolve_ext_ref(entry, target): | |
| """pr-<N> branch on the ext repo, else the manifest's sandbox ref for PR runs, else the F´ base branch name, else the manifest default.""" | |
| candidates = [f"pr-{target['pr']}" if target["pr"] else None, entry.get("pr_ref") if target["pr"] else None, target["base_ref"]] | |
| for c in candidates: | |
| if c and ext.exists(f"/repos/{entry['repository']}/branches/{q(c)}"): | |
| return c | |
| return entry.get("default_ref", "main") |
(best-effort fix; verify before applying)
cc @LeStarch @thomas-bc @bitWarrior — low-confidence finding, please confirm.
There was a problem hiding this comment.
Accepted for the led-blinker pilot, as a maintainer decision rather than a fix: the dispatched runs go to GitHub-hosted runners of an example repository that holds no secrets and grants contents: read only; nasa/fprime requires maintainer approval for workflow runs from all outside collaborators, so every fork head that gets dispatched has been looked at; and the worst realistic outcome is corrupted CI results or artifacts in that repository, recoverable by clearing its caches. Before a second repository is added to the manifest: dispatched runs will neither restore nor save Actions caches (change on the external side), and any repository with self-hosted or hardware runners follows the rule recorded in the PR description (no PR-head code on hardware, permissions: {}, no secrets, no caches, ephemeral runners).
| # Deliberately trivial. This workflow runs under the repository's normal `pull_request` | ||
| # rules (including "require approval for workflows from forks"), and its completion is what | ||
| # starts the privileged orchestrator in ext-ci.yml via `workflow_run`. It must never need |
There was a problem hiding this comment.
[Security] could fix The design leans on "require approval for workflows from forks", but GitHub's default setting gates first-time contributors only (general-vulnerability/trust-boundary).
With the default, any returning contributor's push is dispatched into the external repos with no human in the loop — the same exposure as today's pull_request ext-*.yml, now exercised in another org's repo. State the setting this design assumes ("Require approval for all outside collaborators") here and in the PR, or document that the gate is first-time-only and size the external-side hardening for that.
There was a problem hiding this comment.
Fixed in d2045d3: the trigger workflow's header now states the assumed setting ("Require approval for all outside collaborators", which nasa/fprime has enabled); the PR description records it as well.
| if unknown: | ||
| raise RuntimeError("unknown manifest ids: " + ", ".join(unknown)) | ||
| repos = [r for r in repos if r["id"] in ids] | ||
| elif target["pr"]: |
There was a problem hiding this comment.
[Correctness] suggestion Merge-queue runs bypass both the on_pr filter and the paths_ignore skip (correctness-unhandled-case).
(Same lines as the thread above, different issue: merge groups, not manual runs.) Trigger: workflow_run.event == "merge_group" leaves target["pr"] as None (l.164-168), so this branch is skipped and every manifest repo, on_pr: false ones included, is dispatched; changed_files() also returns None there (l.208-209), so docs-only merge groups are built instead of skipped. Consequence: a nightly-only repo with failure_policy: block gates the queue with a check the PR never had. No such entry exists yet and "the queue runs everything" may be intended — if so, say it in the manifest; otherwise treat merge groups like PRs.
| elif target["pr"]: | |
| elif target["pr"] or (EVENT_NAME == "workflow_run" and EVENT["workflow_run"]["event"] == "merge_group"): |
cc @LeStarch @thomas-bc — low-confidence finding, please confirm.
There was a problem hiding this comment.
Intentional for merge groups: a merge group is reviewed code about to land, so every repo runs there, on_pr: false ones included (that is what on_pr means: not on PR heads), and the file list is unknown so nothing is skipped. Noted in the script header.
thomas-bc-autowot
left a comment
thomas-bc-autowot
left a comment
There was a problem hiding this comment.
Automated review summary (run 1)
Per-agent results
Findings open at the reviewed head, by tag; everything ever raised is under Since last run.
| Agent | must fix | suggestion | could fix | future work | outstanding | Verdict |
|---|---|---|---|---|---|---|
| Security Vulnerabilities | 1 | 3 | 1 | 0 | 5 | No-Go |
| Supply Chain / Runner Safety | 0 | 3 | 0 | 0 | 3 | Go |
| F Prime C/C++ Design | — | — | — | — | — | skipped — no touched surface |
| Documentation Currency | 0 | 1 | 1 | 0 | 2 | Go |
| Design | 1 | 1 | 2 | 0 | 4 | No-Go |
| Architecture | — | — | — | — | — | skipped — no touched surface |
| Test Quality | 0 | 0 | 0 | 0 | 0 | Go |
| Correctness | 1 | 2 | 0 | 0 | 3 | No-Go |
| Operational | 2 | 0 | 2 | 0 | 4 | No-Go |
| Maintainability | 0 | 1 | 3 | 0 | 4 | Go |
| CI safety | — | — | — | — | — | No-Go — security has 1 must-fix in category-8: fork-PR builds dispatched to the external repo's default branch get default-branch Actions-cache write scope (cache poisoning of later ext-ci verdicts and of led-blinker CI) |
| Totals | 4 | 7 | 8 | 0 | 19 | No-Go |
Totals count each open thread once at its strictest tag; threads shared by concurrence appear in every sharing lens's row. Operational's second must-fix is the §14 promotion below.
Supply-chain surfaces
| Surface | Outstanding |
|---|---|
| Dependencies | clean |
| Vendored / submodule | clean |
| Build / test infrastructure | clean |
| Workflows / actions / scripts | 3 suggestion — DISPATCH_TOKEN exposed job-wide in ext-ci.yml (step-scope it) |
| Generator output | clean |
| Prompt-injection | clean |
| Review-system integrity | clean |
Outstanding must-fix items (4)
Security Vulnerabilities
- PR builds dispatched onto the external repo's default branch give untrusted F´ code default-branch Actions-cache write scope (cache-tampering; low-confidence, maintainer ping attached) — #297 (comment)
Design
- Human design adjudication required. New privileged CI mechanism (secret, check contract, manifest) lands with two plan decisions unrecorded: dispatch credential (PAT vs GitHub App) and self-hosted-runner trust class — #297 (comment)
Operational
notifyalso silences orchestrator-side faults and non-PR runs (expiredEXT_CI_DISPATCH_TOKEN, nightly breakage), so the job exits 0 and nobody is told (low-confidence, maintainer ping attached) — #297 (comment)- A dispatch whose response is lost is recorded as
dispatch_failedwhile the remote run proceeds untracked and is never cancelled (promoted, see below) — also: Correctness — #297 (comment)
Severity reconciliation (1 promoted)
| Finding | Reviewer tag | Summary tag | Consequence | Link |
|---|---|---|---|---|
Lost dispatch response → dispatch_failed while the remote run proceeds untracked, never cancelled |
could fix (Operational; Correctness concurred at must fix) | must fix | state left inconsistent on an error path: local state says failed, the live remote run is skipped by poll_all() / finalize() |
#297 (comment) |
Retry bound encoded three times (range(5), i == 4, i < 4) |
suggestion | suggestion (not promoted) | fragile-edit hazard; no §14 row | #297 (comment) |
| Second, narrower shape of the per-repo state record | could fix | could fix (not promoted) | latent KeyError for a future consumer; no §14 row |
#297 (comment) |
failure_policy == "notify" rule re-derived at four sites |
could fix | could fix (not promoted) | duplication; no §14 row | #297 (comment) |
| "only ever rendered inside code spans" comment broader than the code | could fix | could fix (not promoted) | in-file code comment, not an sdd / user-manual / Doxygen claim; rationale states no rendering hazard today | #297 (comment) |
CONTRIBUTING.md does not describe the new ext-ci / <id> check |
could fix | could fix (not promoted) | rationale states the existing sentence stays true (incomplete, not false); a CI check is not a ground-facing surface | #297 (comment) |
Manifest schema and external ci.yml contract undocumented |
suggestion | suggestion (not promoted) | missing documentation for a non-ground-facing surface; no §14 row | #297 (comment) |
DISPATCH_TOKEN job-level env reaches checkout / upload-artifact |
suggestion | suggestion (not promoted) | least-privilege hardening; no §14 row | #297 (comment) |
PR-controlled head_branch written to $GITHUB_STEP_SUMMARY outside a code span |
suggestion | suggestion (not promoted) | markdown-injection surface; the contradicted code() contract is an in-file comment, not a §14 documentation surface |
#297 (comment) |
on_pr: false bypassed when a manual run names repos |
suggestion | suggestion (not promoted) | guardrail gap for repos not yet in the manifest; no §14 row | #297 (comment) |
| External CI tests the PR head alone, not the merge commit | could fix | could fix (not promoted) | stated trade-off, no §14 row | #297 (comment) |
| Release PRs read the default-branch manifest (Design concurrence on the Devin Review thread) | could fix | could fix (not promoted) | undocumented trade-off; no §14 row | #297 (comment) |
Both timeouts inside the try, so a failing poll never times out |
could fix (Correctness concurred at suggestion) | suggestion (not promoted) | unenforced per-repo timeout, runner held to the job limit; no §14 row — strictest concurring tag used | #297 (comment) |
Cancellation path ignores failure_policy, so a notify repo can block |
could fix | could fix (not promoted) | rare timing case; the cancelled conclusion reflects what happened, no §14 row |
#297 (comment) |
| Fork-approval gate is first-time-contributor-only by default | could fix | could fix (not promoted) | the assumed setting is stated in the PR description, not an sdd / user-manual / Doxygen claim | #297 (comment) |
Merge-queue runs bypass on_pr and paths_ignore |
suggestion | suggestion (not promoted) | consequence conditional on a manifest entry that does not exist yet ("could become") | #297 (comment) |
Merge readiness
Merge readiness: No-Go — 4 outstanding must-fix items across Security Vulnerabilities, Design, Operational and Correctness (one thread shared by Operational and Correctness).
Lenses skipped on this PR
- F Prime C/C++ Design — no touched surface: the PR's changed-file list contains no C or C++ source or header file.
- Architecture — no touched surface: the PR's changed-file list contains no
.fppmodel, no topology or deployment file, and no component source or header.
Four items between this dispatch line and go-for-launch; the ground segment is otherwise clean.
Review findings and resolutions (multi-agent review at 6052882)Full summary review: #297 (review) (8 lenses, 2 routed out as not applicable; CI safety No-Go / merge readiness No-Go as posted, with the dispositions below). Boundary verified clean by every lens that traced it: the orchestrator never runs PR code; neither
Rule for external repositories with self-hosted / hardware runnersCompared with the current
|
…nual runs, readability
…trator faults fail the job
There was a problem hiding this comment.
Note
Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.
Devin Review found 1 new potential issue.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| 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)] |
There was a problem hiding this comment.
🔴 Updated external branches retain stale green checks
When an external branch changes without a new F´ commit, check_succeeded skips it on rerun if its old check passed. The external changes go untested, and the old green check remains on the commit.
Learn more
A Check Run belongs to an F´ commit, but the external workflow runs from a separately selected branch. The external pr-<N> branch can be added or updated without changing the F´ commit. On rerun, resolve_ext_ref would select that new branch state, but the success lookup only examines prior Check Runs on the F´ SHA. The repository is excluded before the branch lookup, so the new external code never runs.
Example: PR #42 passes using external devel. A maintainer creates external pr-42 with a compatibility fix, then reruns the orchestrator for the unchanged F´ commit. The existing success causes pr-42 to be skipped instead of tested.
Recommended fix: Re-dispatch on explicit reruns when external branch state must be retested, or record the selected external ref and immutable external commit SHA with each successful check and reuse success only when those inputs match.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Known limit of the attempt-N shortcut, accepted for now: "Re-run" is meant for flaky or fixed external failures, and a green check is only reused for the same F´ commit. When the external side changed and must be retested (new or updated pr-N branch), start a fresh manual dispatch with repos=<id> and pr_number=N — attempt 1 of a new run always re-dispatches. Recording the external SHA alongside the check can come later if this turns out to be a common case.
…; notify repos never cancelled-red
…atest; 60-min job / 30-min remote timeouts
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… stay inside the 60-min job
… / Orchestrate", "External CI / <id>"
…ot match the orchestrator itself
There was a problem hiding this comment.
Note
Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.
Devin Review found 1 new potential issue.
3 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| # GitHub's own approval gate. | ||
| # The workflow file and the checked-out scripts come from the default branch, never from the PR. | ||
| workflow_run: | ||
| workflows: [ "External CI Trigger" ] |
There was a problem hiding this comment.
🔴 Manual run cancels release PR checks
When a manual External CI run on devel finishes, it launches a self-triggered workflow_run despite the job's path guard. Workflow-level concurrency then cancels an active devel-to-release PR orchestration, leaving its external checks cancelled.
Learn more
Both workflow files use the name External CI. A workflow_run subscription matches the orchestrator's own completed runs as well as approval-gate runs. The job-level path condition skips the extra run's job, but concurrency applies at workflow scope first. A self-triggered run inherits its completed run's branch, so it can cancel an unrelated active run with the same workflow-run concurrency group.
Example: A maintainer starts a manual orchestration on devel while a PR from nasa/fprime:devel to release/1.0 has external checks running. The manual orchestration finishes and triggers an extra workflow run on devel. That extra run cancels the PR orchestration before its job is skipped; the PR's remote run and check can be cancelled.
Recommended fix: Give the approval-gate and orchestrator workflows distinct names, subscribe only to the approval-gate name, and keep the shared check prefix in the manifest if desired. This avoids creating self-triggered runs before the job-level path guard can skip them.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in cf2f06e (pushed just before this comment was re-anchored): the trigger workflow is now named "External CI Trigger", the orchestrator subscribes only to that name, and the path guard is gone, so the orchestrator never matches its own completion and no self-triggered run reaches the workflow-level concurrency group.
…) instead of a PAT Check Runs created with GITHUB_TOKEN are filed into an arbitrary existing Actions check suite on the PR head, so the PR shows them as '<other workflow> / ext-ci / led-blinker'. Creating them with the External CI App's own installation token gives them their own suite. The same App's fprime-community installation replaces EXT_CI_DISPATCH_TOKEN; secrets are EXT_CI_APP_ID and EXT_CI_APP_PRIVATE_KEY. Falls back to GITHUB_TOKEN for the checks while the App is not installed on this repo.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Change Description
Dispatch an external project's own
ci.ymland report the result on the F´ commit, instead of building the project inside nasa/fprime.ext-ci-trigger.yml: trivialpull_request/merge_groupworkflow withpermissions: {}, so GitHub's fork-approval gate applies before anything privileged runs.ext-ci.yml: runs onworkflow_runof the trigger, nightly, onrelease/**pushes and manual dispatch, always from the default branch (never PR code). One Check Run per external repo on the PR head SHA; dispatches the externalci.ymlwith{fprime_repository, fprime_ref, fprime_pr_number, dispatch_id}, polls, finalizes (closes open checks, cancels orphaned remote runs). A re-run re-dispatches every repo..github/actions/ext-ci/orchestrator.py(documented in.github/actions/ext-ci/README.md): standard-library-only Python script doing the above..github/actions/ext-ci/manifest.json: pilot withfprime-community/fprime-workshop-led-blinkeronly,failure_policy: notify(a failure shows as a neutral check and does not block).ext-build-led-blinker.ymlstays until the new check has run in parallel for a while.Credentials: one GitHub App ("External CI", registered under fprime-community: Actions rw, Checks rw, Contents ro, Pull requests ro, no webhook) installed on the external repos and on nasa/fprime; repository secrets
EXT_CI_APP_ID+EXT_CI_APP_PRIVATE_KEY. The workflow mints two 1-hour installation tokens: one scoped to the external repos (dispatch/cancel), one to nasa/fprime (Check Runs, so they get their own "External CI" group in the PR checks list instead of being filed under another workflow, which is whatGITHUB_TOKEN-created checks get). Until the nasa/fprime installation is approved the checks fall back toGITHUB_TOKEN. Dispatched runs havecontents: read, no secrets, ccache off; the action-side pip-cache save is closed by nasa/fprime-actionscache-mode: read(separate PR) — see finding 1 below for what that does not cover.Operation:
pr-<N>branch when it exists, else its default branch (shown in the check title, e.g.success (… @ devel)); the externalci.ymlchecks itself out from that ref and builds against the F´ revision it is given.<App name> / External CI / led-blinkerand the merge box the App avatar +External CI / led-blinker; branch protection matches on the check name.Rationale
Each
ext-*.ymlre-implements an external build on nasa/fprime runners and every external breakage is fixed in many workflows; this leaves nasa/fprime with dispatch/poll/report and one manifest entry per project.Testing/Review Recommendations
Design, evidence and security review: https://github.com/JPL-Devin/fprime-test-ci-fprime/blob/main/docs/ext-ci-dispatch-plan.md. Review focus:
ext-ci.ymlholds the write permissions and the secret but checks out only the default branch; nopull_request_target. Proven end to end from the JPL-Devin fork: orchestrator run https://github.com/JPL-Devin/fprime/actions/runs/41948640203 dispatched https://github.com/fprime-community/fprime-workshop-led-blinker/actions/runs/41948655128 and closed theExternal CI / led-blinkercheck with its result. After merge: a PR touching a non-ignored path should get that check.Review findings and resolutions (multi-agent review at 6052882)
Full summary review: #297 (review) (8 lenses, 2 routed out as not applicable; CI safety No-Go / merge readiness No-Go as posted, with the dispositions below).
Boundary verified clean by every lens that traced it: the orchestrator never runs PR code; none of the tokens (
GITHUB_TOKEN, App installation tokens) is reachable from the PR; manifest and script are read from the default/release branch; only GitHub-authoredworkflow_runpayload fields are trusted; PR resolution is filtered onbase.repo; nopull_request_target; Check Runs can only be written by the orchestrator; concurrency keys are owner-qualified;finalizecloses checks and cancels remote runs under cancellation; the dispatched led-blinker workflow hascontents: readand no secrets. The context this review assumed: nasa/fprime requires approval for workflow runs from all outside collaborators, and the external repositories hold no secrets.ccache: false/cache-mode: read, which only stop the action's own saves. (#297 (comment))design-needs-human-adjudication: a new privileged mechanism (long-lived PAT in a repository secret, external dispatch). (#297 (comment))failure_policy: notifyalso silences orchestrator-side faults and nightly/release runs: the job exits 0, nobody is alerted. (#297 (comment))notifyonly softens the PR check; dispatch/poll errors and non-PR runs (nightly, release, merge queue) fail the job.dispatch_failedwhile the remote run proceeds untracked (not polled, not cancelled). (#297 (comment))dispatch_idrun-name before giving up.DISPATCH_TOKENis job-level env (also visible toactions/checkout/upload-artifact); fork-controlledhead_branchwritten raw to the step summary;on_pr: falsebypassed by a manual run naming the repo pluspr_number.head_branchthroughcode(), honouron_pron manual runs.CONTRIBUTING.mdcheck description, manifest/ci.ymlcontract in the script header).Rule for external repositories with self-hosted / hardware runners
Compared with the current
ext-*.ymlmodel, a dispatched run executes in the external repository's trusted context (default branch,workflow_dispatch) instead of a forkpull_requestcontext: GitHub no longer strips secrets or forces a read-only token, default-branch caches are writable (persistence that survives runner re-imaging), and the App's dispatch token can start runs on those runners without an approval click. Before any such repository is added to the manifest:on_pr: falsefor that entry (hardware testsdevelnightly,release/**and the merge queue only), or at most same-repo heads via a job-levelif: inputs.fprime_repository == 'nasa/fprime'in the external workflow.permissions: {}(orcontents: read), no${{ secrets.* }}, no cache restore or save on dispatched runs, ephemeral/JIT runners, SHA-pinned actions, CODEOWNERS on.github/workflows/including nasa/fprime maintainers.workflow_dispatchitsci.ymlwith anyfprime_ref, outside nasa/fprime's approval gate — the external workflow's own restrictions above are what bound that path, not the orchestrator.on_pr: falseis the recommendation.Future Work
Add the other
fprime-communityrepos as theirci.ymllands; retire the matchingext-*.yml.Contributor Checklist
AI Usage (see policy)
Devin, under maintainer direction, wrote the workflows, script and manifest and ran the prototype and reviews.
AGENTS.md).IAMAI