ci: support PR dependencies via depends-on#19075
Conversation
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
There was a problem hiding this comment.
I think this is a big concern. For now it might be a good idea to forgo this change.
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 |
|
@lupyuen Thanks again — good point about release branches and backports. I've gone with the simplest and safest rule:
if [ -n "$PR_BODY" ] && [ "$GITHUB_BASE_REF" = "master" ]; then
# parse depends-on ...
fiThis avoids the backport problem without adding GitHub API calls or extra permissions. What this means:
One limitation: this checks only the current PR's target branch. Without an API lookup, the workflow does not verify the dependency PR's own base branch, so this feature is intended for the normal master-branch cross-repo dependency case. A stricter version could query each dependency PR's base.ref and require it to match, but that would reintroduce an API call and extra permission surface. For now, restricting depends-on to PRs targeting master seems the simpler and safer trade-off. I've validated this in my test fork with a backport-style PR targeting releases/12.13 whose body contains a copied depends-on: line. The Apply depends-on PRs step is skipped and CI builds against the matching releases/12.13 branch, so the copied dependency is ignored: Validation run: https://github.com/zhn-test/nuttx/actions/runs/27262401865/job/80511061759 Please see the Apply depends-on PRs step in the Fetch-Source job. It is skipped. Any downstream build differences in my fork are unrelated to this depends-on handling. If this approach looks OK, I'll update this PR first. After this PR is reviewed and the approach is agreed, I will submit the matching |
|
@zhangning21 Please hang on to the changes, I would like to hear from other maintainers about:
|
|
I'm not sure we should allow metadata within the PR body. This is a cool idea but maybe it needs to be put on hold for now. |
|
Yep I agree that embedding PR Metadata (e.g.
If we ever need to support PR Metadata in future, we would need a lot more work:
|
|
Hi @zhangning21: I'm sorry that we have to close this PR because:
I'm also sorry for your time wasted in preparing this meticulous PR. Perhaps in future, you could create a NuttX Issue first, then assign it to me for discussion, so we can agree on the best solution? I hope you understand that ASF Infrastructure Team is closely watching our usage of GitHub Actions. They nearly banned NuttX Project twice from using GitHub Actions, due to overuse and security concerns. Thanks :-) |
|
@lupyuen please don't close PR without voting:
Do you have other better method to fix the patch which has the cross-git dependence, which is a must have feature, but doesn't fix for a long time.
the dependence is described in the commit message, why change in commit message doesn't trigger a rebuild?
if the description is wrong, ci doesn't cherry-pick the related patch on apps side, then ci will fail loudly.
If awk/sed isn't good, let's stick to pure shell script or switch to python.
How to verify the cross-git patch correctly is a well-known issue for our ci build system, not a new issue. |
|
@xiaoxiang781216 I'm afraid I can't commit to these enhancements for NuttX CI. I'm already overwhelmed by the maintenance of NuttX CI, keeping it performant and secure, as mandated by ASF Infra Team. From now on: I'm stepping away from all NuttX CI Duties, and focusing instead on my Family Matters. @simbit18 @acassis @cederom @linguini1 @raiden00pl @hartmannathan @jerpelea Sorry I need to take a break from NuttX Project, it's hurting my hypertension. Hope to catch you again in future. Goodbye! |
|
@lupyuen best wishes! Hope to hear from you again soon :) |
Hi @lupyuen that is completely understandable. Take a break and focus on your health and your family! I'm sure you will continue to do great things! I wish you all the best! |
acassis
left a comment
acassis
left a comment
There was a problem hiding this comment.
@zhangning21 could you please add those information you added in the Summary into our CI Documentation: https://nuttx.apache.org/docs/latest/testing/nuttx-ci.html
linguini1
left a comment
linguini1
left a comment
There was a problem hiding this comment.
This seems like a great solution, I think it's really cool and it does solve a huge headache NuttX has had for a while. However, given the concerns from Lup about parsing the PR body, I think before this is merged we should check with Apache Infra about what they think about this solution.
ee49fc4 to
b70fca1
Compare
Allow pull requests targeting master to declare same- and cross-repository dependencies. Parse declarations with a tested Python helper, apply exact dependency commits before the existing build matrix, and rerun heavy CI only when an edited description changes the dependency state. Keep fork builds read-only and use a trusted workflow_run to validate artifacts and post per-build dependency results. Document the supported declaration forms and operational limits. Assisted-by: Kiro:gpt-5.6-sol Signed-off-by: zhangning21 <zhangning21@xiaomi.com>
b70fca1 to
1c399cf
Compare
github-actions
Bot
added
Size: L
|
@linguini1 @acassis this pr is ready for review now. |
|
Thanks @acassis! Done — I've added the Summary details to the CI documentation
The section renders like this on the CI docs page: 
|
Security concerns appear to be addressed (?)
5 participants
Summary
Add opt-in same- and cross-repository pull request dependencies to NuttX CI.
Why this is needed
NuttX and nuttx-apps are built together. For a normal PR,
Fetch-Sourcechecksout the target branch from the changed repository and the corresponding branch
from the other repository. A feature that must change both repositories can
therefore fail in both PRs because neither target branch contains the companion
change yet. This feature lets CI test the exact combined changes before either
PR is merged. It also supports a PR depending on another PR in the same
repository.
Usage
A PR targeting
mastercan declare one dependency per line:Alternatively, multiple dependencies can use a single bracket list:
The Depends-On: marker is case-insensitive; Depends-On:, depends-on:,
and mixed-case spellings are equivalent. This document uses Depends-On: as
the canonical form.
Short references and full
https://github.com/...URLs may be mixed. MultipleDepends-On:lines are supported, and duplicate references are applied once infirst-seen order. Each declaration must be on one line; Markdown bullet
continuation is not supported. Markers in prose,
not-depends-on:, indentedcode, and fenced code blocks are ignored. Only references that strictly match
an allowed GitHub repository and a positive PR number are accepted; other
tokens are ignored. A declaration is
invalidonly when aDepends-On:markeris present but no valid dependency can be parsed.
Fetch-Sourcefetches each declaredpull/<N>/head, records its exact head SHA,checks for common history, and cherry-picks the dependency commits before the
existing build matrix runs.
Implementation
.github/scripts/depends_on.pyprovides a standard-library parser with strictaccepted-reference matching, repository allow-listing, Markdown code-block
handling, ordered de-duplication, structured JSON output, and explicit
ok/invalid/nonestates..github/scripts/test_depends_on.pycontains 40 parser regression tests..github/workflows/build.ymlkeeps fork builds read-only, applies dependenciesonly to PRs targeting
master, and reruns heavy CI after aneditedeventwhen the ordered parsed dependency state or base branch changes.
.github/workflows/depends-on-comment.ymlruns after Build completes. Withoutchecking out or executing fork code, it validates the untrusted report's
structure, repository allow-list, run/current-head binding, and fixed
rendering fields before posting a result. Retrying the same run updates that
run's comment; earlier Build comments remain as history.
Documentation/testing/nuttx-ci.rstdocuments the accepted syntax, behavior,result comments, and operational limits.
The follow-up comment reports one of three outcomes:
ok: each dependency PR and its fetched head SHA (abbreviated in thecomment);
invalid: a marker was found but no valid dependency could be parsed, so nodependency is applied and CI continues with the normal source selection;
failed: a valid dependency could not be applied, with a fixed diagnosticreason. This causes
Fetch-Sourceto fail.Impact and security
Depends-On:retain the existing sourceand build-matrix behavior.
masterapply dependencies. Acopied declaration in a release/backport PR is ignored, preserving the
existing matching-release-branch behavior.
cherry-pick happen once in
Fetch-Source, not once per matrix target. EveryBuild gains one short
Changesgate job, and every completed PR Build startsone short trusted comment job, which exits without commenting when no report
exists. Build now listens for
pull_requesteditedevents. An unrelatedtitle/body edit runs only the gate and does not request cancellation of an
already-running code build, although GitHub may replace an older pending run
in the same concurrency group. Reordering dependencies reruns the matrix.
contents: read. The parser usesno
awk,sed,eval, or third-party Python package. The default-branchworkflow_runis the only part of this change withpull-requests: write;it never checks out fork code. It rejects reports with an invalid schema, repository, dependency number, full
success SHA, or run/current-head binding, and renders only fixed text and
validated values.
applied. A valid dependency causes
Fetch-Sourceto fail if fetch,common-history checking, revision-list generation, or cherry-pick fails.
Defensive report validation and other ordinary job errors can also fail the
job rather than silently producing an untrusted result.
behavior changes.
Intentional limitations
pull/<N>/headat Build time and recordsthe fetched SHA, but does not use the GitHub API to validate the dependency
PR's open/closed state or target branch. The initiating PR must target
master; authors must select compatible dependency PRs.The initiating PR must be updated or its checks rerun to test the new SHA.
the dependency PR and do not prove that its independent checks passed. The
trusted workflow validates an untrusted report for safe posting, but does not
independently attest that the dependency was applied; the comment reflects
the read-only Build workflow's report.
Owners still coordinate merge order and timing.
should update the dependency rather than rely on CI to resolve it.
Testing
Host: macOS locally and Ubuntu GitHub-hosted runners. No target-hardware test
is applicable because this changes GitHub Actions workflows, a parser, and CI
documentation.
Local validation:
python3 -m unittest discover -s .github/scripts -p 'test_depends_on.py':40/40 passed. These tests are currently run explicitly with this command;
this change does not add a separate CI job for them;
py_compile, parser CLI smoke test withapache/*, andgit diff --check:passed;
checked;
testing/nuttx-ci.rst: passed, includingassertions for both declaration forms,
invalidbehavior, and all threecomment outcomes.
GitHub Actions validation:
ChangesandFetch-Sourcejobs passed.End-to-end tests: These cases were run in a public staging fork environment
(
zhn-test/*) using real fork PRs, dependency PRs, GitHub Actions runs,artifacts, and bot comments. The links record those test runs; the upstream
configuration and usage examples above use
apache/nuttxandapache/nuttx-apps.depends-on: zhn-test/nuttx-apps/pull/392bb7d36eddepends-on: zhn-test/nuttx-apps/pull/3depends-on: zhn-test/nuttx-apps/pull/492bb7d36edand39bf4ae20fdepends-on: gitlab.com/zhn-test/nuttx-apps/pull/3invalid; warning comment posted and normal source selection continuesdepends-on: zhn-test/nuttx-apps/pull/999999failed; the dependency PR could not be fetcheddepends-on:declarationdepends-on,not-depends-on:, and fenceddepends-on:examplesdepends-on:followed by two Markdown bulletsinvalid; warning comment postedreleases/ci-test;depends-on: zhn-test/nuttx-apps/pull/3Changesran;Fetch-Sourceand all heavy jobs were skipped