Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing supply chain security and transparency by updating development tool versions and introducing a comprehensive plan for Software Bill of Materials (SBOM) attestation. The changes streamline tool dependency management and lay the groundwork for generating and distributing signed CycloneDX SBOMs for native addon binaries, enabling better auditing and vulnerability response for consumers. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Greptile SummaryThis PR adds a CycloneDX SBOM attestation plan for the native The mise version bump is safe. However, the SBOM plan has two blocking logic issues that must be resolved before implementation:
Additionally, the plan deviates from the project's Confidence Score: 2/5
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| uses: "actions/attest-sbom@10926c72720ffc3f7b666661c8e55b1344e2a365" # v2.4.0 | ||
| with: | ||
| subject-path: "packages/node/package.tar.gz" | ||
| sbom-path: "packages/node/bom.json" | ||
| ``` |
There was a problem hiding this comment.
Attestation subject / consumer verify mismatch
The SBOM is attested against packages/node/package.tar.gz (the npm tarball), but the consumer scenario on line 281 instructs users to verify against node_modules/node-reqwest/dist/node_reqwest.node (the extracted binary). gh attestation verify works by hashing the file you provide and looking up that exact digest. Since package.tar.gz and node_reqwest.node have different SHA-256 digests, verification will always fail for consumers.
Either the attestation subject must be changed to the .node binary (if extractable at build-packages stage), or consumer documentation must instruct verification against the tarball:
| uses: "actions/attest-sbom@10926c72720ffc3f7b666661c8e55b1344e2a365" # v2.4.0 | |
| with: | |
| subject-path: "packages/node/package.tar.gz" | |
| sbom-path: "packages/node/bom.json" | |
| ``` | |
| gh attestation verify \ | |
| package.tar.gz \ | |
| --owner vadimpiven \ | |
| --format json \ | |
| --predicate-type https://cyclonedx.org/bom |
The smoke test on line 220 also calls gh attestation verify on package.tar.gz, which is consistent with the attestation — but this creates a gap between what consumers should actually do (as documented on line 281) and what the attestation can verify.
Prompt To Fix With AI
This is a comment left during a code review.
Path: plans/06-sbom-attestation.md
Line: 159-163
Comment:
**Attestation subject / consumer verify mismatch**
The SBOM is attested against `packages/node/package.tar.gz` (the npm tarball), but the consumer scenario on line 281 instructs users to verify against `node_modules/node-reqwest/dist/node_reqwest.node` (the extracted binary). `gh attestation verify` works by hashing the file you provide and looking up that exact digest. Since `package.tar.gz` and `node_reqwest.node` have different SHA-256 digests, verification will always fail for consumers.
Either the attestation subject must be changed to the `.node` binary (if extractable at `build-packages` stage), or consumer documentation must instruct verification against the tarball:
```suggestion
gh attestation verify \
package.tar.gz \
--owner vadimpiven \
--format json \
--predicate-type https://cyclonedx.org/bom
```
The smoke test on line 220 also calls `gh attestation verify` on `package.tar.gz`, which is consistent with the attestation — but this creates a gap between what consumers should actually do (as documented on line 281) and what the attestation can verify.
How can I resolve this? If you propose a fix, please make it concise.| [tools."github:CycloneDX/cyclonedx-rust-cargo"] | ||
| version_prefix = "cargo-cyclonedx-" | ||
| version = "0.5.7" | ||
| os = ["macos"] | ||
| [tools."github:CycloneDX/cyclonedx-rust-cargo".platforms] | ||
| macos-x64 = { asset_pattern = "*-x86_64-apple-darwin*" } | ||
| macos-arm64 = { asset_pattern = "*-aarch64-apple-darwin*" } | ||
| ``` |
There was a problem hiding this comment.
Critical: cargo-cyclonedx not configured for Linux
The mise.toml snippet restricts installation to os = ["macos"], but the SBOM generation step runs on ubuntu-latest (native runner, not Docker) at line 98. With only macOS in the os list, mise will not install cargo-cyclonedx on the Linux runner, and the CI step will fail with "command not found".
The Remaining TODOs section acknowledges this gap, but it is blocking — the plan cannot be implemented as written. Before implementation, either:
- Add
linuxto theoslist with appropriate platform selectors (e.g.,x86_64-unknown-linux-gnuor musl), or - Replace the mise-based installation with a fallback like
cargo binstallon the Linux runner
Verify which approach is correct and update the mise.toml snippet accordingly.
Prompt To Fix With AI
This is a comment left during a code review.
Path: plans/06-sbom-attestation.md
Line: 71-78
Comment:
**Critical: `cargo-cyclonedx` not configured for Linux**
The `mise.toml` snippet restricts installation to `os = ["macos"]`, but the SBOM generation step runs on `ubuntu-latest` (native runner, not Docker) at line 98. With only macOS in the `os` list, mise will not install `cargo-cyclonedx` on the Linux runner, and the CI step will fail with "command not found".
The Remaining TODOs section acknowledges this gap, but it is blocking — the plan cannot be implemented as written. Before implementation, either:
1. Add `linux` to the `os` list with appropriate platform selectors (e.g., `x86_64-unknown-linux-gnu` or musl), or
2. Replace the mise-based installation with a fallback like `cargo binstall` on the Linux runner
Verify which approach is correct and update the `mise.toml` snippet accordingly.
How can I resolve this? If you propose a fix, please make it concise.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive plan for generating and attesting Software Bill of Materials (SBOMs) for the native addon binaries, a significant enhancement for supply chain security. The plan is well-structured and detailed. My review includes suggestions to refine the plan, focusing on the configuration for cargo-cyclonedx, clarifying the subject of the SBOM attestation to ensure it aligns with consumer needs, and improving the validation step in the local mise task for better developer experience.
| - name: "Attest SBOM" | ||
| uses: "actions/attest-sbom@10926c72720ffc3f7b666661c8e55b1344e2a365" # v2.4.0 | ||
| with: | ||
| subject-path: "packages/node/package.tar.gz" | ||
| sbom-path: "packages/node/bom.json" | ||
| ``` |
There was a problem hiding this comment.
The plan suggests creating an SBOM attestation with subject-path: "packages/node/package.tar.gz". This binds the SBOM to the npm package tarball, which primarily contains JavaScript wrapper code.
However, the SBOM describes the Rust dependencies compiled into the .node binary, which is downloaded separately during postinstall. The goal of SBOM attestation is to provide transparency into the binary's contents. Therefore, the attestation subject should be the .node binary itself.
Since there are multiple binaries for different platforms, you should consider creating attestations for each of them. The actions/attest action supports multiple subjects. The build-packages job would need to download the .node artifacts from the build-addon matrix job to use them as subjects.
This also contradicts the consumer scenario described later in the plan (lines 281-286), which correctly shows a user verifying the attestation against the .node file.
| [tools."github:CycloneDX/cyclonedx-rust-cargo"] | ||
| version_prefix = "cargo-cyclonedx-" | ||
| version = "0.5.7" | ||
| os = ["macos"] | ||
| [tools."github:CycloneDX/cyclonedx-rust-cargo".platforms] | ||
| macos-x64 = { asset_pattern = "*-x86_64-apple-darwin*" } | ||
| macos-arm64 = { asset_pattern = "*-aarch64-apple-darwin*" } | ||
| ``` |
There was a problem hiding this comment.
The proposed mise.toml configuration for cargo-cyclonedx only specifies os = ["macos"]. However, the plan states that the SBOM generation will happen in the build-packages job, which runs on ubuntu-latest. This configuration will prevent cargo-cyclonedx from being installed on the Linux runner.
The plan should be updated to include a configuration for Linux. Since a x86_64-unknown-linux-musl binary is available for cargo-cyclonedx, you could update the configuration like this:
[tools."github:CycloneDX/cyclonedx-rust-cargo"]
version_prefix = "cargo-cyclonedx-"
version = "0.5.7"
os = ["linux", "macos"]
[tools."github:CycloneDX/cyclonedx-rust-cargo".platforms]
linux-x64 = { asset_pattern = "*-x86_64-unknown-linux-musl*" }
macos-x64 = { asset_pattern = "*-x86_64-apple-darwin*" }
macos-arm64 = { asset_pattern = "*-aarch64-apple-darwin*" }This will ensure the tool is available on all necessary developer and CI platforms.
| --spec-version 1.5 \ | ||
| --describe crate | ||
| echo "SBOM written to packages/node/bom.json" | ||
| jq -e '.components | length' packages/node/bom.json |
There was a problem hiding this comment.
The validation step in the proposed mise task, jq -e '.components | length' packages/node/bom.json, will only print the number of components and will not fail the task if the SBOM is empty or malformed. To make this a meaningful validation step, it should check if the number of components is greater than a certain threshold, similar to the CI validation step. This will cause the task to fail if the generated SBOM is not as expected, making the local task's validation consistent with the proposed CI validation and providing a more reliable check for developers.
| jq -e '.components | length' packages/node/bom.json | |
| jq -e '.components | length > 50' packages/node/bom.json |
1 participant
No description provided.