ci: add native GitHub Actions pipeline (squashed from 130-pipe / #1737) - #1803
Conversation
📝 WalkthroughWalkthroughReplaces the GitLab CI pipeline with a new GitHub Actions workflow ( ChangesNIXL GitHub Actions CI Pipeline and Build Toolchain
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 195-199: The "Download manylinux wheels" step only downloads the
dist-build-nixl-manylinux artifact, which covers only x86 CUDA 12.9 wheels.
Either add a comment explaining why only this variant is being scanned if this
limitation is intentional, or modify the workflow to download and scan
additional wheel variants by adding more download-artifact steps or modifying
the artifact name pattern to include CUDA 13 and ARM wheel variants to ensure
comprehensive coverage across different CUDA versions and architectures.
- Line 63: Replace all mutable action tag references (such as `@v4`, `@v2`) with
their full commit SHA hashes in the GitHub Actions workflow file to improve
supply-chain security. For each action reference throughout the workflow, update
the tag to use the complete commit SHA followed by a comment indicating the
version (e.g., `# v4.2.2`). This applies to all 16 action references in the
file, including `actions/checkout` and any other actions used. Verify each SHA
corresponds to the correct version tag before committing.
- Around line 63-65: The actions/checkout@v4 step currently persists git
credentials by default, which could leak sensitive information if the .git
directory is inadvertently included in build artifacts. Add the
persist-credentials: false configuration option to the with section of the
checkout action to disable credential persistence and harden the workflow
security.
- Line 275: The GPU test job runner labels use a hardcoded
`prod-nixl-tester-gpu-v1` prefix instead of respecting the `NIXL_RUNNER_PREFIX`
variable that other jobs use. Replace the hardcoded `prod` prefix in the
`runs-on` field at lines 275, 295, and 318 with the dynamic variable pattern
`${{ vars.NIXL_RUNNER_PREFIX || 'prod' }}-nixl-tester-gpu-v1` to ensure GPU jobs
respect the same runner prefix configuration as all other jobs in the workflow.
- Around line 347-354: The bash command in the ci.yml workflow uses a semicolon
(;) after the uv pip install command for wheel installation, which allows
subsequent commands to execute even if the installation fails. Replace the
semicolon with && after the line containing uv pip install
/workspace/nixl*314*whl /workspace/nixl-*none-any.whl to ensure the Python
scripts (expanded_two_peers.py execution) only run if the wheel installation
succeeds, preventing misleading test failures that mask actual installation
issues.
- Around line 23-53: Add a concurrency block to the GitHub Actions workflow to
prevent duplicate runs when multiple commits are pushed quickly or when PRs are
re-labeled. After the permissions section and before or within the env section,
add a top-level concurrency configuration that groups runs by pull request
number for PR events (using github.event.pull_request.number) and by branch/tag
reference for push events (using github.ref), with cancel-in-progress set to
true to automatically cancel previous runs in the same group when a new run
starts.
- Around line 462-471: The docker run command uses single quotes within the bash
-c string for the cargo publish command, which prevents shell variable expansion
of ARTIFACTORY_CARGO_TOKEN and ARTIFACTORY_URL. To fix this, change the single
quotes to double quotes around the --token argument value and the --index
argument value so that the variables referenced in those strings
(ARTIFACTORY_CARGO_TOKEN and ARTIFACTORY_URL) will be properly expanded by the
inner bash shell. Additionally, add ARTIFACTORY_URL to the docker run command's
environment variable list using the -e flag (similar to how
ARTIFACTORY_CARGO_TOKEN is already being passed). Remember to use backslash
escaping (like \$) for variables that should be passed literally through the
GitHub Actions runner to the container's bash shell where they will then be
expanded using the provided environment variables.
- Around line 390-397: The ARTIFACTORY_PYPI_TOKEN and ARTIFACTORY_URL secrets
are being expanded by the host shell and embedded directly in the docker
container command string, making them visible via docker inspect. Move these
secrets into docker environment variables by adding -e flags to the docker
create command for ARTIFACTORY_PYPI_TOKEN and ARTIFACTORY_URL, then reference
them as environment variables (with $ prefix) within the bash command string
passed to the jf rt upload command. This ensures the actual secret values are
only visible inside the container execution context, not in the command string
itself.
- Line 162: The `--os "ubuntu24"` parameter at line 162 in the CI workflow is
being passed unconditionally to Docker build commands, but it is only meaningful
when using `contrib/Dockerfile` for the primary Ubuntu-based build. The
`Dockerfile.manylinux` used by manylinux matrix entries does not declare or use
an OS build argument, making this parameter unused for those builds. Modify the
workflow to conditionally include the `--os "ubuntu24"` parameter only when the
build is using `contrib/Dockerfile`, and omit it for manylinux builds that use
`Dockerfile.manylinux`. This can be achieved by adding a conditional check based
on the dockerfile being used or the build matrix configuration.
In @.github/workflows/stg-nixl-build.yml:
- Around line 6-8: Update the workflow comments in the stg-nixl-build.yml file
to accurately reflect the current image source configuration. The comment block
describing the manylinux-amd job (and similar comments in the 81-84 region)
incorrectly references GitLab registry pulls and GITLAB_REGISTRY_* variables,
but the jobs now use public PyPA/NGC image sources. Replace the outdated
references to GitLab registry pulls with accurate descriptions of the public
PyPA/NGC inputs that are actually being used in the workflow, ensuring the
documentation matches the actual implementation to prevent confusion during
future debugging and onboarding.
- Line 25: Replace all mutable version tags with immutable commit SHAs across
the workflow file. Specifically, update the uses directives for actions/checkout
(lines 25, 74, 118), actions/upload-artifact (line 63), and
aws-actions/amazon-ecr-login (line 79) by replacing the version tag syntax
(e.g., `@v4`, `@v2`) with the full commit SHA for each action (e.g.,
`@1d7c41aa604b5073b0ab45fa3fbab74f474f5304`). This ensures deterministic CI
behavior and eliminates supply-chain security risks from mutable tag references.
- Around line 25-27: The `actions/checkout@v4` action persists credentials in
git config by default, which poses a security risk on self-hosted runners used
by all three jobs. Add `persist-credentials: false` to the with section of each
`actions/checkout@v4` usage (appearing at lines 25-27, 74-76, and 118-120) to
disable credential persistence and reduce token exposure on the self-hosted
runner environments.
- Around line 10-13: The workflow currently triggers on both workflow_dispatch
and pull_request events, and the manylinux-amd job pushes images to ECR
unconditionally, creating a security risk where untrusted PR code can push to
external infrastructure. Add a conditional to the manylinux-amd job (or the ECR
push step specifically) to only execute the ECR image push when the workflow is
triggered by workflow_dispatch. Use a GitHub Actions conditional expression that
checks the github.event_name context variable to ensure ECR pushes only occur
from trusted, manually dispatched workflows and not from pull requests.
In @.github/workflows/stg-nixl-smoke.yml:
- Around line 7-11: The pull_request trigger in the workflow is allowing
untrusted external PRs to execute on privileged self-hosted runners without
proper security gates. Add a condition to restrict pull_request execution to
only same-repository PRs by using a conditional that checks if the pull request
head repository matches the current repository
(github.event.pull_request.head.repo.full_name == github.repository), or
alternatively restrict pull_request-triggered jobs to only run on trusted
environments while keeping workflow_dispatch available for manual reruns on the
self-hosted runners.
- Line 27: The aws-actions/amazon-ecr-login action is using a mutable version
tag (v2) which is vulnerable to tag retargeting attacks. Replace the `@v2` tag
with the full 40-character commit SHA of that release to pin it to an immutable
commit, optionally adding the version tag as a comment for reference. This
ensures the workflow uses a specific, immutable version of the action rather
than one that could be redirected.
In `@contrib/build-container.sh`:
- Around line 98-105: The --cuda-version option has been added to the argument
parsing logic but is missing from the help documentation. Locate the show_help()
function in the script and add documentation for the --cuda-version option to
the help output. Ensure the entry is consistent in formatting and style with the
other command-line options already documented in the help text.
In `@contrib/Dockerfile.manylinux`:
- Around line 58-60: The FROM statement pulling the PyPA manylinux base image is
not pinned to a specific version, which can result in non-reproducible builds
since the image is a rolling release. Update the FROM instruction for
quay.io/pypa/manylinux_2_28_${ARCH} to include a digest pin by appending
`@sha256`: followed by the specific digest hash of the desired image version. This
ensures that the same image is pulled consistently across builds, preventing
unexpected toolchain or package changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: e2b22977-88cb-4535-a7a6-ee03349d3207
📒 Files selected for processing (7)
.github/actionlint.yaml.github/workflows/ci.yml.github/workflows/stg-nixl-build.yml.github/workflows/stg-nixl-smoke.ymlcontrib/Dockerfile.manylinuxcontrib/build-container.shmeson.build
| outputs: | ||
| version: ${{ steps.compute.outputs.version }} | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Consider pinning actions to SHA hashes for supply-chain security.
All action references use mutable tags (@v4, @v2). Pinning to full commit SHAs prevents malicious tag modifications. Example for actions/checkout:
- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2This applies to all 16 action references flagged by static analysis. Tools like Dependabot or Renovate can auto-update pinned hashes.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 63-65: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 63-63: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml at line 63, Replace all mutable action tag
references (such as `@v4`, `@v2`) with their full commit SHA hashes in the
GitHub Actions workflow file to improve supply-chain security. For each action
reference throughout the workflow, update the tag to use the complete commit SHA
followed by a comment indicating the version (e.g., `# v4.2.2`). This applies to
all 16 action references in the file, including `actions/checkout` and any other
actions used. Verify each SHA corresponds to the correct version tag before
committing.
Source: Linters/SAST tools
| --wheel-base "${{ matrix.whl_base }}" \ | ||
| --python-versions "${WHL_PYTHON_VERSIONS}" \ | ||
| --tag "${IMAGE_NAME}" \ | ||
| --os "ubuntu24" \ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how --os flag is used in build-container.sh
rg -n -A5 '\-\-os\)' contrib/build-container.sh
rg -n 'OS=' contrib/build-container.shRepository: ai-dynamo/nixl
Length of output: 300
🏁 Script executed:
#!/bin/bash
# Get context around line 162 in ci.yml
sed -n '150,175p' .github/workflows/ci.yml
# Check the matrix definition and how --os is used
sed -n '88,165p' .github/workflows/ci.yml | grep -n "matrix\|dockerfile\|--os\|ubuntu\|manylinux"Repository: ai-dynamo/nixl
Length of output: 2839
🏁 Script executed:
#!/bin/bash
# Get the full matrix definition
sed -n '88,130p' .github/workflows/ci.ymlRepository: ai-dynamo/nixl
Length of output: 2026
🏁 Script executed:
#!/bin/bash
# Check how OS is used in build-container.sh
rg -n '\$OS' contrib/build-container.sh
cat -n contrib/build-container.sh | sed -n '235,245p'Repository: ai-dynamo/nixl
Length of output: 686
🏁 Script executed:
#!/bin/bash
# Check if OS build arg is used in Dockerfile.manylinux
rg -n '\$OS\|ARG OS' contrib/Dockerfile.manylinux
# Check the regular Dockerfile too
rg -n '\$OS\|ARG OS' contrib/DockerfileRepository: ai-dynamo/nixl
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check if OS argument is defined in Dockerfiles
grep -n "ARG OS" contrib/Dockerfile.manylinux contrib/Dockerfile
# Check the full content for OS usage
head -50 contrib/Dockerfile.manylinux | cat -n
head -50 contrib/Dockerfile | cat -nRepository: ai-dynamo/nixl
Length of output: 5423
🏁 Script executed:
#!/bin/bash
# Check if OS is used in Dockerfile.manylinux at all
grep -n "OS" contrib/Dockerfile.manylinux
# Check what the OS==ubuntu22 check does in build-container.sh
sed -n '166,180p' contrib/build-container.shRepository: ai-dynamo/nixl
Length of output: 481
The --os "ubuntu24" parameter is unused in Dockerfile.manylinux and can be omitted for manylinux builds.
The manylinux matrix entries use nvcr.io/nvidia/cuda:*-ubi8 base images, but Dockerfile.manylinux does not declare or use the OS build argument, making --os "ubuntu24" harmless but unnecessary for those builds. The parameter is only meaningful for the primary Ubuntu-based build (contrib/Dockerfile), where it is appropriate. Consider making the parameter conditional on the Dockerfile being used to avoid passing unused arguments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml at line 162, The `--os "ubuntu24"` parameter at
line 162 in the CI workflow is being passed unconditionally to Docker build
commands, but it is only meaningful when using `contrib/Dockerfile` for the
primary Ubuntu-based build. The `Dockerfile.manylinux` used by manylinux matrix
entries does not declare or use an OS build argument, making this parameter
unused for those builds. Modify the workflow to conditionally include the `--os
"ubuntu24"` parameter only when the build is using `contrib/Dockerfile`, and
omit it for manylinux builds that use `Dockerfile.manylinux`. This can be
achieved by adding a conditional check based on the dockerfile being used or the
build matrix configuration.
| - name: Download manylinux wheels | ||
| uses: actions/download-artifact@v4 | ||
| with: | ||
| name: dist-build-nixl-manylinux | ||
| path: dist |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Scan only covers x86 CUDA 12.9 wheels.
The scan downloads only dist-build-nixl-manylinux artifacts. CUDA 13 and ARM wheel variants are not scanned. If this is intentional (same source code, different binary targets), consider adding a comment. Otherwise, consider scanning all variants or at least one per CUDA major version.
🧰 Tools
🪛 zizmor (1.26.1)
[error] 196-196: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 195 - 199, The "Download manylinux
wheels" step only downloads the dist-build-nixl-manylinux artifact, which covers
only x86 CUDA 12.9 wheels. Either add a comment explaining why only this variant
is being scanned if this limitation is intentional, or modify the workflow to
download and scan additional wheel variants by adding more download-artifact
steps or modifying the artifact name pattern to include CUDA 13 and ARM wheel
variants to ensure comprehensive coverage across different CUDA versions and
architectures.
| - uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat .github/workflows/stg-nixl-build.ymlRepository: ai-dynamo/nixl
Length of output: 5056
🌐 Web query:
GitHub Actions checkout v4 persist-credentials security self-hosted runners best practice
💡 Result:
Best practice for GitHub Actions checkout@v4 on self-hosted runners (security focused): 1) Disable credential persistence unless you truly need it Use persist-credentials: false so checkout does not configure the token/SSH key in the local git config for later steps. The checkout action explicitly documents that the token is persisted in the local git config and that you can opt out with persist-credentials: false. [1][2] Example: steps: - uses: actions/checkout@v4 with: persist-credentials: false 2) Also treat self-hosted runners as high risk; isolate and harden them GitHub’s security guidance for self-hosted runners says they “do not have guarantees around running in ephemeral clean virtual machines, and can be persistently compromised by untrusted code,” so they “should almost never be used for public repositories.” [3] Accordingly: - Only allow trusted repositories/workflows onto a given runner (use runner groups/boundaries rather than “all repositories”). [3] - Prefer ephemeral/container-based patterns when possible; GitHub guidance emphasizes the lack of ephemeral isolation on self-hosted runners and the need for additional controls. [3] - Don’t assume compromise is limited to a single job: because self-hosted environments can persist, assume a compromised workflow could persist access or exfiltrate secrets. [3] 3) Be explicit about least-privilege token permissions Even if persist-credentials is disabled, the workflow still has access to GITHUB_TOKEN; GitHub recommends setting GITHUB_TOKEN permissions appropriately (the checkout README notes recommendations for GITHUB_TOKEN permissions when using checkout). [1] 4) Understand what checkout does today (context for persist-credentials) The checkout README states that when persistence is enabled, the auth token is persisted so scripts can run authenticated git commands, and it is removed during post-job cleanup; setting persist-credentials: false opts out. [1][2] Additionally, newer checkout versions improved credential handling by storing persisted credentials under $RUNNER_TEMP rather than directly in.git/config (helpful context, but opt-out is still the safer default). [4] Bottom line - For most self-hosted runner security posture, use actions/checkout@v4 with persist-credentials: false unless your job genuinely requires persisted git auth for later steps. [1][2][5] - Harden runner usage (trusted repos only, least-privilege, isolation/ephemerality) because self-hosted runners can be persistently compromised and do not have the same isolation guarantees as GitHub-hosted runners. [3]
Citations:
- 1: https://github.com/actions/checkout/?tab=readme-ov-file
- 2: https://github.com/actions/checkout/blob/v4/README.md
- 3: https://docs.github.com/en/actions/reference/security/secure-use
- 4: https://github.com/actions/checkout/blob/main/README.md
- 5: https://cheatsheetseries.owasp.org/cheatsheets/GitHub_Actions_Security_Cheat_Sheet.html
Disable persisted checkout credentials on self-hosted runners.
All three jobs run on self-hosted runners (stg-nixl-builder-amd-v1 and stg-nixl-builder-arm-v1). By default, actions/checkout persists credentials in the local git config, which increases token exposure on self-hosted environments. Set persist-credentials: false to mitigate this risk.
Suggested hardening
- uses: actions/checkout@v4
with:
fetch-depth: 0
+ persist-credentials: falseApplies to lines: 25-27, 74-76, 118-120
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 25-27: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 25-25: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/stg-nixl-build.yml around lines 25 - 27, The
`actions/checkout@v4` action persists credentials in git config by default,
which poses a security risk on self-hosted runners used by all three jobs. Add
`persist-credentials: false` to the with section of each `actions/checkout@v4`
usage (appearing at lines 25-27, 74-76, and 118-120) to disable credential
persistence and reduce token exposure on the self-hosted runner environments.
Source: Linters/SAST tools
| on: | ||
| # Runs as a PR check from the PR's head branch (works without the file being on | ||
| # main). workflow_dispatch is kept for manual reruns once this lands on main. | ||
| pull_request: | ||
| workflow_dispatch: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "stg-nixl-smoke.yml" -type fRepository: ai-dynamo/nixl
Length of output: 97
🏁 Script executed:
cat .github/workflows/stg-nixl-smoke.ymlRepository: ai-dynamo/nixl
Length of output: 1282
Restrict PR-triggered execution on self-hosted staging runners.
This workflow executes on self-hosted runners (stg-nixl-builder-amd-v1, stg-nixl-builder-arm-v1) for pull_request events. Without a same-repo gate, untrusted PRs can run on privileged infrastructure.
Suggested hardening
jobs:
smoke-amd:
+ if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository
runs-on: stg-nixl-builder-amd-v1
smoke-arm:
+ if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository
runs-on: stg-nixl-builder-arm-v1🧰 Tools
🪛 zizmor (1.26.1)
[warning] 7-11: insufficient job-level concurrency limits (concurrency-limits): workflow is missing concurrency setting
(concurrency-limits)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/stg-nixl-smoke.yml around lines 7 - 11, The pull_request
trigger in the workflow is allowing untrusted external PRs to execute on
privileged self-hosted runners without proper security gates. Add a condition to
restrict pull_request execution to only same-repository PRs by using a
conditional that checks if the pull request head repository matches the current
repository (github.event.pull_request.head.repo.full_name == github.repository),
or alternatively restrict pull_request-triggered jobs to only run on trusted
environments while keeping workflow_dispatch available for manual reruns on the
self-hosted runners.
| - name: Docker (dind sidecar) works | ||
| run: docker info | ||
| - name: IRSA -> ECR auth works | ||
| uses: aws-actions/amazon-ecr-login@v2 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and inspect the relevant lines
cat -n .github/workflows/stg-nixl-smoke.yml | sed -n '20,45p'Repository: ai-dynamo/nixl
Length of output: 780
🌐 Web query:
GitHub Actions best practices pinning commit SHA vs tag security supply chain integrity
💡 Result:
Pinning GitHub Actions to a full-length commit SHA is considered a critical best practice for supply chain integrity [1][2][3]. Because Git tags and branches are mutable—meaning they can be moved, deleted, or re-pointed to different code by an attacker or a compromised account—referencing them in workflows poses a significant security risk [1][4][5]. Key takeaways regarding SHA pinning versus tags: 1. Immutability: A full-length commit SHA is immutable. Once a commit exists, its hash cannot change without changing its content, ensuring that your workflow executes the exact code you expect every time [4][6]. 2. Risk of Tag Retargeting: If a bad actor gains access to an action's repository, they can update existing tags to point to a malicious commit [1][7]. Any workflow referencing that action by the old tag will automatically begin running the malicious code without the user's knowledge [2][5]. 3. Policy Enforcement: GitHub now supports organization- and repository-level policies that enforce SHA pinning [8][4]. When enabled, workflows that reference actions via tags or branches will fail, preventing the use of unpinned dependencies [8][4]. This policy often checks the entire dependency tree, including internal references within actions [4]. 4. Best Practices for Implementation: - Use Full SHAs: Always use the full 40-character commit SHA [9][3]. - Maintain Readability: Include the corresponding version tag as a comment in your YAML file (e.g., uses: actions/checkout@3df4ab11... # v4.0.0) [2][3]. - Automate Updates: Use dependency management tools like Renovate (with the helpers:pinGitHubActionDigests preset) or pinact to automate the process of updating SHAs while keeping your workflow files maintained [2][7][5]. - Beyond Pinning: SHA pinning is necessary but not always sufficient. Some actions may pull mutable external dependencies at runtime (such as Docker images tagged:latest or unpinned internal dependencies). Periodically audit workflows for these "unpinnable" actions and consider alternatives like vendoring the code or using local actions [5]. - Security Policies: Enable GitHub's built-in policies to block non-SHA-pinned actions, and use tools like Zizmor or custom linting to enforce these standards across all repositories [2][5]. In summary, while pinning to a tag is more convenient, it is inherently less secure than pinning to a commit SHA. Organizations should prioritize moving to SHA-based pinning to mitigate the risk of supply chain attacks [2][4].
Citations:
- 1: https://docs.github.com/en/actions/reference/security/secure-use
- 2: https://architecture.lullabot.com/adr/20260505-pin-github-actions-to-hash/
- 3: https://aquilax.ai/blog/github-actions-security-hardening
- 4: https://www.romainlespinasse.dev/posts/github-actions-commit-sha-pinning/
- 5: https://mikael.barbero.tech/blog/post/2026-03-24-stop-trusting-mutable-references/
- 6: https://www.stepsecurity.io/blog/pinning-github-actions-for-enhanced-security-a-complete-guide
- 7: https://emmer.dev/blog/pin-your-github-actions-to-protect-against-mutability/
- 8: https://github.blog/changelog/2025-08-15-github-actions-policy-now-supports-blocking-and-sha-pinning-actions/
- 9: https://docs.ozarksecuritylabs.com/supply-chain/tier-1-baseline/sha-pin-actions/
Pin GitHub Actions to immutable commit SHAs for supply chain integrity.
aws-actions/amazon-ecr-login@v2 is tag-pinned, which is mutable and allows tag retargeting attacks. Pin to the full 40-character commit SHA instead. Optionally include the version tag as a comment for reference.
Suggested fix
- - name: IRSA -> ECR auth works
- uses: aws-actions/amazon-ecr-login@v2
+ - name: IRSA -> ECR auth works
+ uses: aws-actions/amazon-ecr-login@<commit-sha> # v2Replace <commit-sha> with the full commit SHA of the desired release.
🧰 Tools
🪛 zizmor (1.26.1)
[error] 27-27: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/stg-nixl-smoke.yml at line 27, The
aws-actions/amazon-ecr-login action is using a mutable version tag (v2) which is
vulnerable to tag retargeting attacks. Replace the `@v2` tag with the full
40-character commit SHA of that release to pin it to an immutable commit,
optionally adding the version tag as a comment for reference. This ensures the
workflow uses a specific, immutable version of the action rather than one that
could be redirected.
Source: Linters/SAST tools
| # Manylinux build base — official PyPA image (public on quay.io; velonix also has | ||
| # a quay ECR pull-through). AlmaLinux 8 / glibc 2.28 => manylinux_2_28 wheels. | ||
| FROM quay.io/pypa/manylinux_2_28_${ARCH} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider pinning the manylinux base image to a digest or date tag for reproducibility.
The PyPA manylinux images are rolling releases. While the _2_28 suffix guarantees glibc ABI, the toolchain and packages inside can change between pulls, potentially causing non-reproducible builds or unexpected breakage.
PyPA publishes digest-tagged images; you can pin to a specific SHA256 digest:
FROM quay.io/pypa/manylinux_2_28_${ARCH}`@sha256`:<digest>Alternatively, accept this as a known tradeoff for always having the latest manylinux toolchain patches.
🧰 Tools
🪛 Hadolint (2.14.0)
[warning] 60-60: Always tag the version of an image explicitly
(DL3006)
🪛 Trivy (0.69.3)
[warning] 60-60: ':latest' tag used
Specify a tag in the 'FROM' statement for image 'quay.io/pypa/manylinux_2_28_x86_64'
Rule: DS-0001
(IaC/Dockerfile)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contrib/Dockerfile.manylinux` around lines 58 - 60, The FROM statement
pulling the PyPA manylinux base image is not pinned to a specific version, which
can result in non-reproducible builds since the image is a rolling release.
Update the FROM instruction for quay.io/pypa/manylinux_2_28_${ARCH} to include a
digest pin by appending `@sha256`: followed by the specific digest hash of the
desired image version. This ensures that the same image is pulled consistently
across builds, preventing unexpected toolchain or package changes.
Source: Linters/SAST tools
2b3def6 to
2c30d97
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 27: The workflow is triggered on version tags (v*) at line 27, but the
release job conditions at lines 50, 185, 228, 266, and 309 only check for
dispatch input or release/** branches, causing release scan and publish jobs to
be skipped on tag pushes. Update all the if condition statements in the release
jobs to also check for version tag triggers by adding a condition that detects
when the workflow was triggered by a tag matching the v* pattern, ensuring the
release jobs execute properly when tags are pushed.
- Around line 169-170: The docker cp commands for copying the dist directory and
nixl_install directory (lines 169-170 and also line 177) both use || true which
masks build failures. Remove the || true clause from these docker cp commands so
that if the build outputs are missing due to a broken wheel build, the job will
properly fail instead of silently succeeding. This ensures broken builds are
caught immediately rather than passing the CI job and failing downstream.
- Around line 23-25: The pull_request trigger in the on section of the workflow
currently allows untrusted code from forks to execute on privileged self-hosted
runners with AWS/ECR access. Add conditional checks to jobs that use self-hosted
runners to ensure they only execute on trusted PRs from the repository itself,
not from forks. Specifically, add a condition checking that the pull request
head repository matches the current repository (using
github.event.pull_request.head.repo.full_name == github.repository) to any jobs
running on ARC self-hosted runners. Apply this same trust gate pattern to all
locations in the workflow where self-hosted runners are used (including the
additional locations at lines 57-58 and 92-93) to prevent untrusted PR code from
accessing privileged AWS/ECR operations.
In `@contrib/build-container.sh`:
- Around line 244-247: The docker build command is using --provenance and --sbom
flags which are only supported by the BuildKit backend, but the invocation uses
plain docker build instead of docker buildx build. Either change the docker
build invocation to docker buildx build if the CI infrastructure has
BuildKit/buildx available, or remove the --provenance=false --sbom=false flags
from the command line and rely on environment variables to control attestations
on buildx-compatible systems only. Verify with your CI team that the chosen
approach is compatible with the build infrastructure before merging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 5e5e1653-26af-4f99-9ca6-9199c4e4c85b
📒 Files selected for processing (5)
.github/actionlint.yaml.github/workflows/ci.ymlcontrib/Dockerfile.manylinuxcontrib/build-container.shmeson.build
| on: | ||
| pull_request: | ||
| push: |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Add a trust gate before running PR code on privileged self-hosted runners.
pull_request currently executes repository code on ARC self-hosted runners that also perform IRSA-backed AWS/ECR operations. Without a fork/trust guard, untrusted PR code can run in a privileged environment.
🔒 Minimal hardening pattern
jobs:
version:
+ if: ${{ github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository }}
runs-on: ${{ vars.NIXL_RUNNER_PREFIX || 'prod' }}-nixl-builder-amd-v1
@@
build:
+ if: ${{ github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository }}
needs: version
runs-on: ${{ vars.NIXL_RUNNER_PREFIX || 'prod' }}-nixl-builder-${{ matrix.runner }}-v1Also applies to: 57-58, 92-93
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 23-37: insufficient job-level concurrency limits (concurrency-limits): workflow is missing concurrency setting
(concurrency-limits)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 23 - 25, The pull_request trigger in
the on section of the workflow currently allows untrusted code from forks to
execute on privileged self-hosted runners with AWS/ECR access. Add conditional
checks to jobs that use self-hosted runners to ensure they only execute on
trusted PRs from the repository itself, not from forks. Specifically, add a
condition checking that the pull request head repository matches the current
repository (using github.event.pull_request.head.repo.full_name ==
github.repository) to any jobs running on ARC self-hosted runners. Apply this
same trust gate pattern to all locations in the workflow where self-hosted
runners are used (including the additional locations at lines 57-58 and 92-93)
to prevent untrusted PR code from accessing privileged AWS/ECR operations.
| pull_request: | ||
| push: | ||
| branches: [main, 'release/**'] | ||
| tags: ['v*'] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Tag pushes are not treated as release builds, so scan/upload release jobs are skipped.
Line 27 triggers on v* tags, but Line 50 and the release job if conditions only check dispatch input or release/** branches. A tag push will build but not run release scan/publish paths.
✅ Suggested fix
- RELEASE_BUILD: ${{ github.event.inputs.release_build == true || github.event.inputs.release_build == 'true' || startsWith(github.ref, 'refs/heads/release/') }}
+ RELEASE_BUILD: ${{ github.event.inputs.release_build == true || github.event.inputs.release_build == 'true' || startsWith(github.ref, 'refs/heads/release/') || startsWith(github.ref, 'refs/tags/v') }}
- if: ${{ github.event.inputs.security_scan == 'true' || github.event.inputs.release_build == 'true' || startsWith(github.ref, 'refs/heads/release/') }}
+ if: ${{ github.event.inputs.security_scan == 'true' || github.event.inputs.release_build == 'true' || startsWith(github.ref, 'refs/heads/release/') || startsWith(github.ref, 'refs/tags/v') }}
- if: ${{ github.event.inputs.release_build == 'true' || startsWith(github.ref, 'refs/heads/release/') }}
+ if: ${{ github.event.inputs.release_build == 'true' || startsWith(github.ref, 'refs/heads/release/') || startsWith(github.ref, 'refs/tags/v') }}Also applies to: 50-50, 185-185, 228-228, 266-266, 309-309
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml at line 27, The workflow is triggered on version
tags (v*) at line 27, but the release job conditions at lines 50, 185, 228, 266,
and 309 only check for dispatch input or release/** branches, causing release
scan and publish jobs to be skipped on tag pushes. Update all the if condition
statements in the release jobs to also check for version tag triggers by adding
a condition that detects when the workflow was triggered by a tag matching the
v* pattern, ensuring the release jobs execute properly when tags are pushed.
| docker cp "$CN:/workspace/nixl/dist" ./dist || true | ||
| docker cp "$CN:/usr/local/nixl" ./nixl_install || true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don’t mask missing build outputs in the build job.
docker cp for dist is best-effort (|| true) and artifact upload is warning-only. A broken wheel build can pass this job and fail much later downstream.
🛠️ Tighten failure behavior
- docker cp "$CN:/workspace/nixl/dist" ./dist || true
+ docker cp "$CN:/workspace/nixl/dist" ./dist
@@
- if-no-files-found: warn
+ if-no-files-found: errorAlso applies to: 177-177
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 169 - 170, The docker cp commands for
copying the dist directory and nixl_install directory (lines 169-170 and also
line 177) both use || true which masks build failures. Remove the || true clause
from these docker cp commands so that if the build outputs are missing due to a
broken wheel build, the job will properly fail instead of silently succeeding.
This ensures broken builds are caught immediately rather than passing the CI job
and failing downstream.
| # --provenance/--sbom default to true under buildx and add a slow attestation-manifest | ||
| # export/unpack step at push time (which was tipping the ARM build over the job timeout). | ||
| # CI images don't need attestations, so disable them. | ||
| docker build --provenance=false --sbom=false --platform linux/$ARCH -f $DOCKER_FILE $BUILD_ARGS $TAG $NO_CACHE $BUILD_CONTEXT |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical
🧩 Analysis chain
🌐 Web query:
Are --provenance and --sbom flags supported by classic docker build, or only by docker buildx build / BuildKit backend?
💡 Result:
The --provenance and --sbom flags are supported exclusively by the docker buildx build command, which utilizes the BuildKit backend [1][2]. These flags are not supported by the classic docker build command. The --provenance and --sbom options are shorthand for --attest=type=provenance and --attest=type=sbom, respectively [1][3][4]. These features rely on the build attestation capabilities introduced within BuildKit, which allow for the generation of metadata such as Software Bill of Materials (SBOM) and SLSA-compliant provenance records during the build process [2][4][5]. Because these attestations are intrinsically linked to the BuildKit build architecture and the in-toto attestation framework, they are not available in the legacy builder [2][5].
Citations:
- 1: https://docs.docker.com/reference/cli/docker/buildx/build
- 2: https://docs.docker.com/build/metadata/attestations/
- 3: https://docs.docker.com/build/metadata/attestations/slsa-provenance
- 4: https://docs.docker.com/build/metadata/attestations/sbom/
- 5: https://www.docker.com/blog/generate-sboms-with-buildkit/
🏁 Script executed:
# Find CI configuration files
find . -type f \( -name ".gitlab-ci.yml" -o -name "*.yml" -o -name "*.yaml" \) | grep -E "(\.github|\.gitlab|ci|CI)" | head -20Repository: ai-dynamo/nixl
Length of output: 865
🏁 Script executed:
# Check for docker-related configuration and CI setup
git ls-files | grep -E "(docker|ci|github|gitlab)" | head -30Repository: ai-dynamo/nixl
Length of output: 1198
🏁 Script executed:
# Look for the build-container.sh script context and any CI references
find . -type f -name "build-container.sh" -exec dirname {} \;Repository: ai-dynamo/nixl
Length of output: 162
🏁 Script executed:
# Read the build-container.sh script to understand context
head -100 ./contrib/build-container.sh
tail -20 ./contrib/build-container.shRepository: ai-dynamo/nixl
Length of output: 4112
🏁 Script executed:
# Check the Jenkins Jenkinsfile for docker backend configuration
cat ./.ci/jenkins/pipeline/Jenkinsfile | head -100Repository: ai-dynamo/nixl
Length of output: 2816
🏁 Script executed:
# Search for "buildx" or "docker build" references in CI configs
rg "buildx|docker\s+build" ./.github/workflows/ ./.ci/jenkins/ --type yaml --type sh -A 2Repository: ai-dynamo/nixl
Length of output: 152
🏁 Script executed:
# Check if there are any docker version checks in the codebase
rg "docker.*version|BuildKit" ./contrib/ ./.ci/ --type sh -A 2Repository: ai-dynamo/nixl
Length of output: 152
🏁 Script executed:
# Final check: Look for any docker build invocations to see if buildx is used anywhere
rg "docker\s+(buildx\s+)?build" . --type sh --type yaml -B 2 -A 2 | head -50Repository: ai-dynamo/nixl
Length of output: 1710
🏁 Script executed:
# Check if there's any docker version requirement documented in CI or build docs
rg "docker.*version|require.*docker" . --type yaml --type sh --type md -i -B 1 -A 1 | head -40Repository: ai-dynamo/nixl
Length of output: 1133
Fix docker build invocation to use BuildKit/buildx or remove unsupported flags.
The --provenance and --sbom flags are only supported by the BuildKit backend (via docker buildx build or Docker 23+ with buildx explicitly enabled). The script currently uses plain docker build, which will reject these flags on any Docker Engine without BuildKit support, causing the build to fail.
Either:
- Change the invocation to
docker buildx build(if CI runners have buildx available), or - Remove
--provenance=false --sbom=falseand handle attestations via environment variables only (for buildx-compatible systems)
Confirm the CI infrastructure has BuildKit/buildx available before merging this change to avoid breaking builds on classic Docker.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 247-247: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 247-247: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 247-247: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 247-247: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 247-247: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 247-247: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contrib/build-container.sh` around lines 244 - 247, The docker build command
is using --provenance and --sbom flags which are only supported by the BuildKit
backend, but the invocation uses plain docker build instead of docker buildx
build. Either change the docker build invocation to docker buildx build if the
CI infrastructure has BuildKit/buildx available, or remove the
--provenance=false --sbom=false flags from the command line and rely on
environment variables to control attestations on buildx-compatible systems only.
Verify with your CI team that the chosen approach is compatible with the build
infrastructure before merging.
Brings nixl's CI to main as a GitHub Actions pipeline, replacing the GitLab mirror+trigger flow: - .github/workflows/ci.yml — version + 5-way build matrix (build-nixl + manylinux x86/arm x cuda12.9/13), wheel security scan, and Artifactory wheel/crate upload. RC generation runs on a push to a release/** branch (i.e. a PR merged into release/<x.y.z> builds + uploads the RC), or a manual workflow_dispatch. Test/verify jobs are omitted for now (re-add once green). - contrib/Dockerfile.manylinux — Option B (public PyPA manylinux_2_28 + NGC CUDA, no GitLab base) plus the INFINIA libs stage and its build deps. - contrib/build-container.sh — --cuda-version + provenance/sbom flags. - meson.build — build_tests gate fix; .github/actionlint.yaml. Hardening (CodeRabbit review): add a concurrency group; fail the build job on empty output (if-no-files-found: error + no-wheels check) instead of masking it; pass the Artifactory token to the upload containers via env (drop set -x / no token in the traced command); persist-credentials: false on checkout; document --cuda-version in build-container.sh help. Squashed from the 130-pipe work (#1737); the 1.3.0 version bump and INFINIA source are already on main (#1738). The GitLab nSpect/scan trigger comes in the follow-up release-pipeline change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2c30d97 to
f6330b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 239-240: The publish jobs at lines 239, 278, and 319 currently
only depend on the build job, allowing them to publish before the scan-wheels
job completes or even if it fails. Update the needs field in each of these
publish job definitions to include both build and scan-wheels as dependencies,
ensuring the scanning completes successfully before any publishing occurs.
- Around line 45-47: The concurrency group at line 46 uses only the branch name
(github.head_ref), which causes unintended cancellations when multiple PRs are
created from the same source branch. Modify the concurrency group key to include
a PR-specific identifier (such as the pull request number from
github.event.pull_request.number) in addition to or instead of just the branch
reference, ensuring that each PR has a unique concurrency key while still
allowing cancellation of previous runs within the same PR.
In `@contrib/build-container.sh`:
- Around line 44-46: Add validation logic after CUDA_VERSION and BASE_IMAGE_TAG
are set to ensure they are compatible with each other. Extract the CUDA version
from BASE_IMAGE_TAG (for example, extract "13.0" from "13.0.1-devel-ubi8") and
compare it against the provided CUDA_VERSION value. If they do not match, log a
clear error message and exit the script before proceeding with the build,
preventing the creation of mismatched artifacts due to typos or configuration
errors.
In `@contrib/Dockerfile.manylinux`:
- Around line 67-70: The CUDA_VERSION argument is not validated when declared,
which allows the image build to proceed through expensive steps before failing
later on an invalid CUDA index. Add a validation check immediately after the ENV
CUDA_VERSION=${CUDA_VERSION} line that verifies CUDA_VERSION is not empty and
exits with an error message if it is missing, ensuring the build fails fast when
this required argument is omitted.
- Around line 188-216: Add SHA256 verification before extracting the downloaded
tarballs for liburing and OpenLDAP. After downloading the
liburing-${LIBURING_VERSION}.tar.gz file (where LIBURING_VERSION=2.6), add a
SHA256 hash verification step before the tar extraction command. Similarly,
after downloading openldap-${OPENLDAP_VERSION}.tgz (where
OPENLDAP_VERSION=2.6.8), add SHA256 verification before extraction. Use the echo
and sha256sum commands to verify the checksums match the official upstream
releases, following the same pattern already established in the Dockerfile for
other packages like rustup. Obtain the official SHA256 hashes from the upstream
project release pages and include them in the verification steps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 3c2f9f84-3940-4e19-8e56-4fe601a1f7ae
📒 Files selected for processing (5)
.github/actionlint.yaml.github/workflows/ci.ymlcontrib/Dockerfile.manylinuxcontrib/build-container.shmeson.build
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.head_ref || github.ref }} | ||
| cancel-in-progress: ${{ github.event_name == 'pull_request' }} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use a PR-unique concurrency key to avoid cross-PR cancellations.
At Line 46, github.head_ref is only the branch name. Two PRs with the same source branch name can cancel each other unintentionally.
🛠️ Proposed fix
concurrency:
- group: ${{ github.workflow }}-${{ github.head_ref || github.ref }}
+ group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
cancel-in-progress: ${{ github.event_name == 'pull_request' }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 45 - 47, The concurrency group at line
46 uses only the branch name (github.head_ref), which causes unintended
cancellations when multiple PRs are created from the same source branch. Modify
the concurrency group key to include a PR-specific identifier (such as the pull
request number from github.event.pull_request.number) in addition to or instead
of just the branch reference, ensuring that each PR has a unique concurrency key
while still allowing cancellation of previous runs within the same PR.
| needs: build | ||
| if: ${{ github.event.inputs.release_build == 'true' || startsWith(github.ref, 'refs/heads/release/') }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Make wheel/crate publishing depend on the scan job.
At Lines 239/278/319, publish jobs depend only on build, so releases can publish before scan-wheels finishes (or even if it fails).
🛠️ Proposed fix
upload-x86-wheels:
- needs: build
+ needs: [build, scan-wheels]
@@
upload-arm-wheels:
- needs: build
+ needs: [build, scan-wheels]
@@
upload-crates:
- needs: build
+ needs: [build, scan-wheels]Also applies to: 278-279, 319-320
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 239 - 240, The publish jobs at lines
239, 278, and 319 currently only depend on the build job, allowing them to
publish before the scan-wheels job completes or even if it fails. Update the
needs field in each of these publish job definitions to include both build and
scan-wheels as dependencies, ensuring the scanning completes successfully before
any publishing occurs.
| # CUDA toolkit version (e.g. 12.9 / 13.0). Option B (manylinux on public PyPA base) | ||
| # no longer inherits this from the base image's ENV, so it must be passed in. | ||
| CUDA_VERSION=${CUDA_VERSION:-} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate that --cuda-version matches the selected CUDA image tag.
CUDA_VERSION drives the Torch CUDA index and cu12/cu13 wheel split, while BASE_IMAGE_TAG selects the actual toolkit copied into the image. A typo like --base-image-tag 13.0.1-devel-ubi8 --cuda-version 12.9 would produce mismatched artifacts.
🐛 Proposed guard before building
+if [[ -n "$CUDA_VERSION" && "$BASE_IMAGE_TAG" =~ ^([0-9]+\.[0-9]+) && "$CUDA_VERSION" != "${BASH_REMATCH[1]}" ]]; then
+ error "ERROR: --cuda-version ($CUDA_VERSION) must match --base-image-tag ($BASE_IMAGE_TAG)." ""
+fi
+
BUILD_ARGS+=" --build-arg CUDA_VERSION=$CUDA_VERSION"Also applies to: 232-232
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contrib/build-container.sh` around lines 44 - 46, Add validation logic after
CUDA_VERSION and BASE_IMAGE_TAG are set to ensure they are compatible with each
other. Extract the CUDA version from BASE_IMAGE_TAG (for example, extract "13.0"
from "13.0.1-devel-ubi8") and compare it against the provided CUDA_VERSION
value. If they do not match, log a clear error message and exit the script
before proceeding with the build, preventing the creation of mismatched
artifacts due to typos or configuration errors.
| # CUDA version (e.g. 12.9 / 13.0) — was provided as an ENV by the old GitLab base; | ||
| # now passed explicitly. Used for the torch cuXXX index and the cu12/cu13 wheel split. | ||
| ARG CUDA_VERSION | ||
| ENV CUDA_VERSION=${CUDA_VERSION} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fail fast when CUDA_VERSION is not supplied.
CUDA_VERSION is required later for the PyTorch CUDA index and wheel split; if it is omitted, this image can run many expensive build steps before failing on an invalid cu index. Validate it immediately after the arg is materialized.
🐛 Proposed fail-fast check
ARG CUDA_VERSION
ENV CUDA_VERSION=${CUDA_VERSION}
+RUN test -n "${CUDA_VERSION}" || { \
+ echo "CUDA_VERSION build arg is required, e.g. 12.9 or 13.0" >&2; \
+ exit 1; \
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contrib/Dockerfile.manylinux` around lines 67 - 70, The CUDA_VERSION argument
is not validated when declared, which allows the image build to proceed through
expensive steps before failing later on an invalid CUDA index. Add a validation
check immediately after the ENV CUDA_VERSION=${CUDA_VERSION} line that verifies
CUDA_VERSION is not empty and exits with an error message if it is missing,
ensuring the build fails fast when this required argument is omitted.
| RUN cd /tmp && \ | ||
| LIBURING_VERSION=2.6 && \ | ||
| wget -q "https://github.com/axboe/liburing/archive/refs/tags/liburing-${LIBURING_VERSION}.tar.gz" && \ | ||
| tar -xzf "liburing-${LIBURING_VERSION}.tar.gz" && \ | ||
| cd "liburing-liburing-${LIBURING_VERSION}" && \ | ||
| ./configure --prefix=/usr/local && \ | ||
| make -j"${NPROC:-$(nproc)}" && \ | ||
| make install && \ | ||
| echo "/usr/local/lib" > /etc/ld.so.conf.d/usrlocal.conf && \ | ||
| echo "/usr/local/lib64" >> /etc/ld.so.conf.d/usrlocal.conf && \ | ||
| ldconfig && \ | ||
| rm -rf /tmp/liburing* | ||
|
|
||
| # OpenLDAP 2.6 client libs: EL8 ships 2.4 (libldap-2.4.so.2); libred_client needs the | ||
| # 2.6 soname libldap.so.2 (+ liblber.so.2). Build client libs only, against openssl3. | ||
| RUN cd /tmp && \ | ||
| OPENLDAP_VERSION=2.6.8 && \ | ||
| wget -q "https://www.openldap.org/software/download/OpenLDAP/openldap-release/openldap-${OPENLDAP_VERSION}.tgz" && \ | ||
| tar -xzf "openldap-${OPENLDAP_VERSION}.tgz" && \ | ||
| cd "openldap-${OPENLDAP_VERSION}" && \ | ||
| ./configure --prefix=/usr/local --enable-shared --disable-static --disable-slapd \ | ||
| --without-cyrus-sasl --with-tls=openssl \ | ||
| CPPFLAGS="-I/usr/local/openssl3/include" \ | ||
| LDFLAGS="-L/usr/local/openssl3/lib64 -L/usr/local/openssl3/lib" && \ | ||
| make depend && \ | ||
| make -j"${NPROC:-$(nproc)}" && \ | ||
| make install && \ | ||
| ldconfig && \ | ||
| rm -rf /tmp/openldap-${OPENLDAP_VERSION}* |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
curl -fsSL \
-o "$tmpdir/liburing-2.6.tar.gz" \
"https://github.com/axboe/liburing/archive/refs/tags/liburing-2.6.tar.gz"
curl -fsSL \
-o "$tmpdir/openldap-2.6.8.tgz" \
"https://www.openldap.org/software/download/OpenLDAP/openldap-release/openldap-2.6.8.tgz"
sha256sum "$tmpdir/liburing-2.6.tar.gz" "$tmpdir/openldap-2.6.8.tgz"Repository: ai-dynamo/nixl
Length of output: 200
🏁 Script executed:
# First, verify the Dockerfile exists and check the relevant lines
git ls-files | grep -i dockerfileRepository: ai-dynamo/nixl
Length of output: 437
🏁 Script executed:
# Attempt to compute hashes using Python's hashlib
python3 - <<'PY'
import urllib.request
import hashlib
import tempfile
import os
urls = {
'liburing': 'https://github.com/axboe/liburing/archive/refs/tags/liburing-2.6.tar.gz',
'openldap': 'https://www.openldap.org/software/download/OpenLDAP/openldap-release/openldap-2.6.8.tgz'
}
with tempfile.TemporaryDirectory() as tmpdir:
for name, url in urls.items():
try:
print(f"\nFetching {name}...")
filepath = os.path.join(tmpdir, f"{name}.tar.gz")
urllib.request.urlretrieve(url, filepath)
# Compute SHA256
sha256_hash = hashlib.sha256()
with open(filepath, 'rb') as f:
for chunk in iter(lambda: f.read(4096), b''):
sha256_hash.update(chunk)
print(f"{name}: {sha256_hash.hexdigest()}")
except Exception as e:
print(f"Error fetching {name}: {e}")
PYRepository: ai-dynamo/nixl
Length of output: 502
🏁 Script executed:
# Check the exact lines mentioned in the review
sed -n '188,216p' contrib/Dockerfile.manylinuxRepository: ai-dynamo/nixl
Length of output: 1542
🏁 Script executed:
# Check the purpose of the Dockerfile - look at the beginning
head -50 contrib/Dockerfile.manylinuxRepository: ai-dynamo/nixl
Length of output: 3059
🏁 Script executed:
# Search for any existing verification/checksum patterns in the Dockerfile
grep -n -i "sha256\|checksum\|verify\|sign" contrib/Dockerfile.manylinuxRepository: ai-dynamo/nixl
Length of output: 430
🏁 Script executed:
# Look for all wget/curl downloads to see which are unverified
grep -n "wget\|curl" contrib/Dockerfile.manylinux | head -20Repository: ai-dynamo/nixl
Length of output: 1289
🏁 Script executed:
# Check context around rustup verification to see the pattern
sed -n '335,345p' contrib/Dockerfile.manylinuxRepository: ai-dynamo/nixl
Length of output: 861
Add SHA256 verification for liburing and OpenLDAP tarballs before extraction.
These source downloads are built directly into the CI image used for release artifacts. Add SHA256 verification before extraction to prevent installation of replaced upstream assets or compromised downloads. The Dockerfile already uses this pattern for other packages (e.g., rustup); apply it consistently here.
Proposed verification pattern
+ARG LIBURING_SHA256="<verified-liburing-2.6-sha256>"
RUN cd /tmp && \
LIBURING_VERSION=2.6 && \
wget -q "https://github.com/axboe/liburing/archive/refs/tags/liburing-${LIBURING_VERSION}.tar.gz" && \
+ echo "${LIBURING_SHA256} liburing-${LIBURING_VERSION}.tar.gz" | sha256sum -c - && \
tar -xzf "liburing-${LIBURING_VERSION}.tar.gz" && \
cd "liburing-liburing-${LIBURING_VERSION}" && \
./configure --prefix=/usr/local && \
make -j"${NPROC:-$(nproc)}" && \
make install && \
@@
+ARG OPENLDAP_SHA256="<verified-openldap-2.6.8-sha256>"
RUN cd /tmp && \
OPENLDAP_VERSION=2.6.8 && \
wget -q "https://www.openldap.org/software/download/OpenLDAP/openldap-release/openldap-${OPENLDAP_VERSION}.tgz" && \
+ echo "${OPENLDAP_SHA256} openldap-${OPENLDAP_VERSION}.tgz" | sha256sum -c - && \
tar -xzf "openldap-${OPENLDAP_VERSION}.tgz" && \
cd "openldap-${OPENLDAP_VERSION}" && \Fill the hash values using checksums from the official upstream releases.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RUN cd /tmp && \ | |
| LIBURING_VERSION=2.6 && \ | |
| wget -q "https://github.com/axboe/liburing/archive/refs/tags/liburing-${LIBURING_VERSION}.tar.gz" && \ | |
| tar -xzf "liburing-${LIBURING_VERSION}.tar.gz" && \ | |
| cd "liburing-liburing-${LIBURING_VERSION}" && \ | |
| ./configure --prefix=/usr/local && \ | |
| make -j"${NPROC:-$(nproc)}" && \ | |
| make install && \ | |
| echo "/usr/local/lib" > /etc/ld.so.conf.d/usrlocal.conf && \ | |
| echo "/usr/local/lib64" >> /etc/ld.so.conf.d/usrlocal.conf && \ | |
| ldconfig && \ | |
| rm -rf /tmp/liburing* | |
| # OpenLDAP 2.6 client libs: EL8 ships 2.4 (libldap-2.4.so.2); libred_client needs the | |
| # 2.6 soname libldap.so.2 (+ liblber.so.2). Build client libs only, against openssl3. | |
| RUN cd /tmp && \ | |
| OPENLDAP_VERSION=2.6.8 && \ | |
| wget -q "https://www.openldap.org/software/download/OpenLDAP/openldap-release/openldap-${OPENLDAP_VERSION}.tgz" && \ | |
| tar -xzf "openldap-${OPENLDAP_VERSION}.tgz" && \ | |
| cd "openldap-${OPENLDAP_VERSION}" && \ | |
| ./configure --prefix=/usr/local --enable-shared --disable-static --disable-slapd \ | |
| --without-cyrus-sasl --with-tls=openssl \ | |
| CPPFLAGS="-I/usr/local/openssl3/include" \ | |
| LDFLAGS="-L/usr/local/openssl3/lib64 -L/usr/local/openssl3/lib" && \ | |
| make depend && \ | |
| make -j"${NPROC:-$(nproc)}" && \ | |
| make install && \ | |
| ldconfig && \ | |
| rm -rf /tmp/openldap-${OPENLDAP_VERSION}* | |
| ARG LIBURING_SHA256="<verified-liburing-2.6-sha256>" | |
| RUN cd /tmp && \ | |
| LIBURING_VERSION=2.6 && \ | |
| wget -q "https://github.com/axboe/liburing/archive/refs/tags/liburing-${LIBURING_VERSION}.tar.gz" && \ | |
| echo "${LIBURING_SHA256} liburing-${LIBURING_VERSION}.tar.gz" | sha256sum -c - && \ | |
| tar -xzf "liburing-${LIBURING_VERSION}.tar.gz" && \ | |
| cd "liburing-liburing-${LIBURING_VERSION}" && \ | |
| ./configure --prefix=/usr/local && \ | |
| make -j"${NPROC:-$(nproc)}" && \ | |
| make install && \ | |
| echo "/usr/local/lib" > /etc/ld.so.conf.d/usrlocal.conf && \ | |
| echo "/usr/local/lib64" >> /etc/ld.so.conf.d/usrlocal.conf && \ | |
| ldconfig && \ | |
| rm -rf /tmp/liburing* | |
| # OpenLDAP 2.6 client libs: EL8 ships 2.4 (libldap-2.4.so.2); libred_client needs the | |
| # 2.6 soname libldap.so.2 (+ liblber.so.2). Build client libs only, against openssl3. | |
| ARG OPENLDAP_SHA256="<verified-openldap-2.6.8-sha256>" | |
| RUN cd /tmp && \ | |
| OPENLDAP_VERSION=2.6.8 && \ | |
| wget -q "https://www.openldap.org/software/download/OpenLDAP/openldap-release/openldap-${OPENLDAP_VERSION}.tgz" && \ | |
| echo "${OPENLDAP_SHA256} openldap-${OPENLDAP_VERSION}.tgz" | sha256sum -c - && \ | |
| tar -xzf "openldap-${OPENLDAP_VERSION}.tgz" && \ | |
| cd "openldap-${OPENLDAP_VERSION}" && \ | |
| ./configure --prefix=/usr/local --enable-shared --disable-static --disable-slapd \ | |
| --without-cyrus-sasl --with-tls=openssl \ | |
| CPPFLAGS="-I/usr/local/openssl3/include" \ | |
| LDFLAGS="-L/usr/local/openssl3/lib64 -L/usr/local/openssl3/lib" && \ | |
| make depend && \ | |
| make -j"${NPROC:-$(nproc)}" && \ | |
| make install && \ | |
| ldconfig && \ | |
| rm -rf /tmp/openldap-${OPENLDAP_VERSION}* |
🧰 Tools
🪛 Hadolint (2.14.0)
[warning] 188-188: Use WORKDIR to switch to a directory
(DL3003)
[info] 203-203: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 203-203: Use WORKDIR to switch to a directory
(DL3003)
🪛 Trivy (0.69.3)
[warning] 188-199: 'RUN cd ...' to change directory
RUN should not be used to change directory: 'cd /tmp && LIBURING_VERSION=2.6 && wget -q "https://github.com/axboe/liburing/archive/refs/tags/liburing-${LIBURING_VERSION}.tar.gz" && tar -xzf "liburing-${LIBURING_VERSION}.tar.gz" && cd "liburing-liburing-${LIBURING_VERSION}" && ./configure --prefix=/usr/local && make -j"${NPROC:-$(nproc)}" && make install && echo "/usr/local/lib" > /etc/ld.so.conf.d/usrlocal.conf && echo "/usr/local/lib64" >> /etc/ld.so.conf.d/usrlocal.conf && ldconfig && rm -rf /tmp/liburing*'. Use 'WORKDIR' statement instead.
Rule: DS-0013
(IaC/Dockerfile)
[warning] 203-216: 'RUN cd ...' to change directory
RUN should not be used to change directory: 'cd /tmp && OPENLDAP_VERSION=2.6.8 && wget -q "https://www.openldap.org/software/download/OpenLDAP/openldap-release/openldap-${OPENLDAP_VERSION}.tgz" && tar -xzf "openldap-${OPENLDAP_VERSION}.tgz" && cd "openldap-${OPENLDAP_VERSION}" && ./configure --prefix=/usr/local --enable-shared --disable-static --disable-slapd --without-cyrus-sasl --with-tls=openssl CPPFLAGS="-I/usr/local/openssl3/include" LDFLAGS="-L/usr/local/openssl3/lib64 -L/usr/local/openssl3/lib" && make depend && make -j"${NPROC:-$(nproc)}" && make install && ldconfig && rm -rf /tmp/openldap-${OPENLDAP_VERSION}*'. Use 'WORKDIR' statement instead.
Rule: DS-0013
(IaC/Dockerfile)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contrib/Dockerfile.manylinux` around lines 188 - 216, Add SHA256 verification
before extracting the downloaded tarballs for liburing and OpenLDAP. After
downloading the liburing-${LIBURING_VERSION}.tar.gz file (where
LIBURING_VERSION=2.6), add a SHA256 hash verification step before the tar
extraction command. Similarly, after downloading
openldap-${OPENLDAP_VERSION}.tgz (where OPENLDAP_VERSION=2.6.8), add SHA256
verification before extraction. Use the echo and sha256sum commands to verify
the checksums match the official upstream releases, following the same pattern
already established in the Dockerfile for other packages like rustup. Obtain the
official SHA256 hashes from the upstream project release pages and include them
in the verification steps.
… (#1803) Brings nixl's CI to main as a GitHub Actions pipeline, replacing the GitLab mirror+trigger flow: - .github/workflows/ci.yml — version, 5-way build matrix (build-nixl + manylinux x86/arm × cuda12.9/13), scan, CPU tests, Artifactory wheel/crate upload, gated on tag/release. - contrib/Dockerfile.manylinux — Option B (public PyPA manylinux_2_28 + NGC CUDA, no GitLab base) plus the INFINIA libs stage and its build deps. - contrib/build-container.sh — --cuda-version + provenance/sbom flags. - meson.build — build_tests gate fix; .github/actionlint.yaml. Squashed from the 130-pipe work (#1737); the 1.3.0 version bump and INFINIA source are already on main (#1738), so this is the CI-pipeline delta only. The stg-nixl-* staging-validation workflows are included pending a decision to drop them; ci.yml still carries the TEMP ci-release-test label gate. ## What? _Describe what this PR is doing._ ## Why? _Justification for the PR. If there is an existing issue/bug, please reference it. For bug fixes, the 'Why?' and 'What?' can be merged into a single item._ ## How? _It is optional, but for complex PRs, please provide information about the design, architecture, approach, etc._ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Migrated continuous integration from GitLab to GitHub Actions with PR/push/tag triggers, security scanning, and release-focused artifact publishing. * Added support for CUDA-version selection during image builds and expanded multi-variant (x86_64/ARM, CUDA variants, manylinux) build outputs. * Refreshed the manylinux container build to use public base images, improving toolchain/library readiness for wheel builds. * Adjusted build behavior so test binaries are included when building release test artifacts. * Added repository linting configuration to recognize known self-hosted runner labels. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e / ai-dynamo#1737) (ai-dynamo#1803)" This reverts commit c8e7b78.
…scan) - Add the manual-approval 'release' environment to upload-x86-wheels and upload-arm-wheels (previously only upload-crates/trigger had it, so wheel uploads published to Artifactory without the approval gate). - Wire the security_scan workflow_dispatch input into the GitLab trigger's ENABLE_WHEEL_SCAN variable (was hardcoded true / input was dead). Defaults true on release/** pushes; on workflow_dispatch it honors the input. (SHA-pinning of actions intentionally not changed — consistent with the prior decision on #1803.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Bump the pinned nvcr.io/nvidia/cuda-dl-base tag from 25.10-cuda13.0-devel-ubuntu24.04 to 26.08-cuda13.4-devel-ubuntu24.04 in both container Dockerfiles. The base image itself is unchanged. benchmark/nixlbench/contrib/build.sh carried its own hardcoded copy of the same tag as a script default, so the pin lived in two places and could silently drift from the Dockerfile it builds. Drop it: BASE_IMAGE/BASE_IMAGE_TAG now default to empty and are only passed as --build-arg when a caller explicitly overrides them, which is already how contrib/build-container.sh behaves. A default run resolves the base image from the Dockerfile ARG, leaving one source of truth. Signed-off-by: NirWolfer <nwolfer@nvidia.com>
1 participant
Brings nixl's CI to main as a GitHub Actions pipeline, replacing the GitLab mirror+trigger flow:
Squashed from the 130-pipe work (#1737); the 1.3.0 version bump and INFINIA source are already on main (#1738), so this is the CI-pipeline delta only. The stg-nixl-* staging-validation workflows are included pending a decision to drop them; ci.yml still carries the TEMP ci-release-test label gate.
What?
Describe what this PR is doing.
Why?
Justification for the PR. If there is an existing issue/bug, please reference it. For
bug fixes, the 'Why?' and 'What?' can be merged into a single item.
How?
It is optional, but for complex PRs, please provide information about the design,
architecture, approach, etc.
Summary by CodeRabbit