ci: support PR dependencies via depends-on#19075
Conversation
This adds a depends-on declaration in the PR description, e.g.: depends-on: [apache/pull/100 apache/nuttx-apps/pull/200] Cross-repo PR dependencies, multiple dependent PRs, and dependencies on other PRs within the same repository can all be combined and supported. Once CI parses the dependencies, it fetches each dependent PR in the corresponding local repository and cherry-picks its commits, so the build runs against the combined code of all involved PRs. The current approach is intentionally conservative: it requires neither a GitHub App nor extra write permissions (which we do not have), making it safe. Its limitation is that it does not write status or comments back to the dependent PRs, i.e. a failing CI cannot mark the dependent PR as failed; when the PRs must be merged together, the PR owners need to coordinate the merge. Signed-off-by: zhangning21 <zhangning21@xiaomi.com>
zhangning21
requested review from
lupyuen,
raiden00pl and
simbit18
as code owners
github-actions
Bot
added
Area: CI
Size: S
|
|
I noticed that you're using an AI Agent for OpenVela. I'm curious if you used it for creating this PR, because this PR doesn't seem to follow the NuttX Contributing Guidelines. Thanks :-) |
| id: gittargets | ||
| shell: bash | ||
| env: | ||
| PR_BODY: ${{ github.event.pull_request.body }} |
There was a problem hiding this comment.
Is this safe? Do we need to escape the PR Body? Otherwise we could have an Injection Attack?
There was a problem hiding this comment.
Thanks for raising this. Yes, the PR body is untrusted input, so we need to be careful here.
The reason we pass the body through an env: variable instead of inlining ${{ github.event.pull_request.body }} directly into the run: script is precisely to avoid script injection. This is the mitigation GitHub documents in Security hardening for GitHub Actions
(https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#using-an-intermediate-environment-variable):
│
│ - With env:, the body is delivered to the step as the value of an environment variable. It is never substituted into the script source, so shell metacharacters in the body (`, $(
), ;, &&, …) are treated as literal data, not executed.
│ - The dangerous form would be inlining it directly, e.g. run: echo "${{ github.event.pull_request.body }}", where a body like "; rm -rf / # would be injected into the script text.
We deliberately do not do that.
│
│ On top of the env: indirection, the body is only ever consumed as quoted data by text-processing tools — echo "$PR_BODY" | grep -oE … — never eval'd or executed. The values we extract are further constrained:
│ - dependencies must match a fixed regex (?:https://github.com/)?apache/nuttx(\?:-apps)?/pull/[0-9]+;
│ - the repo is checked against a 2-entry allow-list (apache/nuttx, apache/nuttx-apps);
│ - the PR number is [0-9]+ only, so the later git fetch origin "pull/${DEP_PR_NUM}/head:…" can't be abused.
│
│ Finally, this workflow runs on pull_request (not pull_request_target), so the job has a read-only GITHUB_TOKEN and no secrets — the blast radius is minimal even in the worst case.
│
│ If helpful, I can switch the echo "$PR_BODY" calls to printf '%s\n' "$PR_BODY" for slightly more robust handling of arbitrary text (purely a robustness nicety, not a security fix).
There was a problem hiding this comment.
@zhangning21 This GitHub Actions Design is very unusual for NuttX CI. If I understand correctly:
- We expect the PR Author to specify inside the PR Body the dependency:
depends-on: apache/nuttx/pull/88888888 - What if the PR Author edits the dependency in the PR Body? Will the dependency be rechecked?
- I'm concerned about parsing the Untrusted Input from the PR Body. Isn't a PR Label a better way to provide the dependency? E.g.
depends-on=nuttx/88888888 - Are there any other projects using this? I wonder if they are also OK with parsing Untrusted Input from the PR Body.
- @simbit18 @linguini1 @cederom Do we think it's a good idea to parse the dependency from the PR Body?
depends-on: apache/nuttx/pull/88888888
There was a problem hiding this comment.
Purpose
│ nuttx and nuttx-apps are built together in CI, and for a normal PR the Fetch-Source job always checks out the master of the other repo. The main problem this solves is cross-repo PR interdependency: when one feature must change both repos, each PR's CI fails because the other repo's master doesn't yet contain the matching change — today the only workaround is to force-merge one side with CI skipped, which risks breaking master. The same mechanism also covers the case where a PR depends on another PR in the same repo. The author declares this in the PR body, e.g. depends-on: [apache/nuttx-apps/pull/XXX], and CI builds the combined code. It's fully opt-in — without a depends-on line, CI behaves exactly as today.
Now to your specific questions:
│
│ 1. Yes, the author specifies the dependency in the PR body.
│
│ 2. "If the author edits the dependency in the body, is it rechecked?"
│ If the author edits only the PR body, it is not rechecked immediately. This follows the current workflow behavior: the existing pull_request trigger does not run CI for PR description edits, only for normal CI-triggering events such as new commits. The dependency will be re-read on the next CI run.
│
│ 3. "Isn't a PR Label better than parsing untrusted body text?"
│ Labels would be more controlled, but they are not very practical here because external contributors usually cannot apply labels to upstream PRs, and dependency values are dynamic PR numbers rather than fixed categories. Using the PR body lets the contributor declare the dependency directly, while the workflow still validates it with a strict allowlist and numeric PR ID.
│
│ 4. "Do other projects parse dependencies from the PR body, and are they OK with the untrusted input?"
│ Yes . A similar approach is used by Zuul CI for cross-project dependencies. Zuul supports a Depends-On: directive, and for GitHub-based projects it is placed in the pull request
description: https://zuul-ci.org/docs/zuul/latest/gating.html#cross-project-dependencies
There was a problem hiding this comment.
The main problem this solves is cross-repo PR interdependency: when one feature must change both repos, each PR's CI fails because the other repo's master doesn't yet contain the matching change — today the only workaround is to force-merge one side with CI skipped, which risks breaking master.
I'm not sure if my Fellow Maintainers agree with me: But here's what I think about Breaking Changes that require both NuttX Repo and NuttX Apps Repo to be in sync...
Breaking Changes need to be carefully and manually managed. I expect the PR Author to test the changes in their own NuttX Repo and NuttX Apps Repo, and provide evidence that All NuttX Builds were successful. Then CI Team needs to standby and make sure that both NuttX Repo and NuttX Apps Repo are merged at the same time.
If we allow PR Authors to specify which version of NuttX / NuttX Apps to build: We might forget to do the manual checking and the simultaneous merging. And when NuttX / NuttX Apps repos go out of sync, we will have lots more problems :-(
|
Hi NuttX Admins: Please don't click "Approve Workflows To Run", I have concerns about the Safety of the GitHub Actions: |
@lupyuen The I'll update the PR to follow the guidelines and ping you again. Apologies for the rough first pass, and thanks for the careful review! |
| ARRAY_DEPS=$(echo "$PR_BODY" | grep -oE 'depends-on:[[:space:]]*\[[^]]+\]' | head -1) || true | ||
| if [ -n "$ARRAY_DEPS" ]; then | ||
| DEPS=$(echo "$ARRAY_DEPS" | grep -oE '(https://github.com/)?apache/nuttx(-apps)?/pull/[0-9]+') || true | ||
| else | ||
| DEPS=$(echo "$PR_BODY" | grep -oE 'depends-on:[[:space:]]*(https://github.com/)?apache/nuttx(-apps)?/pull/[0-9]+' | sed 's/depends-on:[[:space:]]*//' | head -1) || true | ||
| fi | ||
|
|
||
| for DEP in $DEPS; do | ||
| DEP=$(echo "$DEP" | sed 's|https://github.com/||') | ||
| DEP_REPO=$(echo "$DEP" | awk -F'/pull/' '{print $1}') | ||
| DEP_PR_NUM=$(echo "$DEP" | awk -F'/pull/' '{print $2}') | ||
|
|
||
| if [[ "$DEP_REPO" != "apache/nuttx" && "$DEP_REPO" != "apache/nuttx-apps" ]]; then | ||
| echo "::warning::Ignoring unsupported dependency repo: $DEP_REPO" | ||
| continue | ||
| fi | ||
|
|
||
| DEPENDS_ON="$DEPENDS_ON ${DEP_REPO}/pull/${DEP_PR_NUM}" | ||
| done | ||
|
|
||
| DEPENDS_ON=$(echo "$DEPENDS_ON" | tr ' ' '\n' | awk 'NF && !a[$0]++' | xargs) |
There was a problem hiding this comment.
Hi NuttX Admins: This script will parse the Untrusted Input from the PR Body to extract the Dependency Info safely, which will prevent Injection Attacks inside the PR Body. I'm afraid the current NuttX CI Team doesn't have sufficient expertise to maintain this, we might introduce Injection Attacks in future.
I strongly suggest that we engage a NuttX Team Member familiar with GitHub Actions Script Security, who will be able to maintain this script, to prevent Injection Attacks in future. We must comply with the Apache Guidelines for GitHub Actions Security: https://infra.apache.org/github-actions-policy.html
Could you show us a working version of this code in your Own NuttX Repo? Also we need the Test Logs for the various test cases thanks!
|
@lupyuen Here's a working version running in my own forks (
Note: cases 4 and 5 are silently ignored (no error, no warning) because the values don't match the dependency regex. If preferred, I can add a warning when a |
|
2 participants
Summary
This PR adds support for cross-repo (and same-repo) PR dependencies to the NuttX CI, driven by a
depends-ondeclaration in the PR description.Why it is needed
nuttxandnuttx-appsare built together in CI. For a normal PR, theFetch-Sourcejob always checks out themasterof the other repo. When a single feature must change both repos, each PR's CI fails because the other repo'smasterdoes not yet contain the matching change. Today the only workaround is to force-merge one side with CI skipped, which risks breakingmaster.What is changed
Only
.github/workflows/build.ymlis modified, in two places inside the existingFetch-Sourcejob:Determine Target Branches— parse an optionaldepends-ondeclaration from the PR body and output it asdepends_on.Apply depends-on PRs— for each declared dependency, fetch the dependent PR'spull/<N>/headinto the corresponding local checkout andcherry-pickits commits, so the build runs against the combined code.How it is used (in the PR description):
depends-on: [apache/nuttx/pull/XXX apache/nuttx-apps/pull/YYY]
https://github.com/...URLs are both accepted and may be mixed; entries are de-duplicated.Impact
Apply depends-on PRsstep is guarded byif: steps.gittargets.outputs.depends_on != ''. PRs without adepends-ondeclaration (the vast majority) behave exactly as today — the step is skipped.grep/awk, no network, sub-second). For PRs that do declare a dependency, the extra work (git fetch --unshallow+git fetch pull/<N>/head+cherry-pick) runs once in the singleFetch-Sourcejob, never multiplied across the target matrix. So there is no measurable runner-budget increase for regular PRs.depends-onis declared. When declared and a dependency conflicts or contains a merge commit,Fetch-Sourcefails fast with a clear message (intended signal to rebase); this only affects the PR that opted in.env:variable (the GitHub-recommended mitigation against script injection) and used only as quoted data forgrep/awk/sed— nevereval'd. Parsed values are constrained by a fixed regex, a 2-entry repo allow-list, and a numeric PR number. The workflow runs onpull_request(read-only token, no secrets).Testing
Build Host: Ubuntu 22.04 (GitHub-hosted
ubuntu-latest)Targets: All targets currently built by NuttX CI (full
build.ymlmatrix).Full-target logs from this PR's own run will be linked once the workflow is
approved to run.
Feature validation (on personal forks):
depends-onApply depends-on PRsskipped; behavior identical to baselinecherry-pick --abort