Conversation
Signed-off-by: Cal Mac Fadden <108666242+calmacfadden@users.noreply.github.com>
📝 WalkthroughWalkthroughThe PR hardens the CI supply chain by adding Yarn guardrails ( ChangesCI Supply-Chain Hardening
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
19-20: ⚡ Quick winAdd
persist-credentials: falseto all checkout steps for supply-chain defense-in-depth.All checkout steps default to persisting git credentials in the runner's git config. For a supply-chain hardening PR, disabling credential persistence reduces the attack surface if a compromised dependency or later step attempts to exfiltrate credentials.
.github/workflows/ci.yml#L19-L20: Addpersist-credentials: falseto yarn-install job checkout..github/workflows/ci.yml#L45-L46: Addpersist-credentials: falseto test job checkout..github/workflows/ci.yml#L80-L81: Addpersist-credentials: falseto size job checkout..github/workflows/ci.yml#L108-L109: Addpersist-credentials: falseto coverage job checkout.Example fix pattern for each checkout step
- name: Checkout repository uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # actions/checkout v4 + with: + persist-credentials: false🤖 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 19 - 20, Add `persist-credentials: false` parameter to all checkout steps in the GitHub Actions workflow for supply-chain security hardening. In `.github/workflows/ci.yml` at lines 19-20 (yarn-install job checkout), lines 45-46 (test job checkout), lines 80-81 (size job checkout), and lines 108-109 (coverage job checkout), add the `persist-credentials: false` option to each `actions/checkout` step to prevent git credentials from persisting in the runner's git config and reduce attack surface if dependencies or subsequent steps are compromised.Source: Linters/SAST tools
14-39: ⚡ Quick winAdd
persist-credentials: falseto checkout step for defense-in-depth.The checkout action at line 20 defaults to persisting git credentials in the runner's git config. In a PR focused on supply-chain hardening, explicitly disabling credential persistence prevents potential credential exfiltration if a compromised dependency or later step accesses the runner's git config.
- name: Checkout repository uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # actions/checkout v4 + with: + persist-credentials: false🤖 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 14 - 39, The checkout action step at the "Checkout repository" task is not explicitly disabling git credential persistence, which leaves the runner vulnerable to credential exfiltration from compromised dependencies. Add a with section to the checkout step and include persist-credentials: false as a configuration option to explicitly prevent git credentials from being persisted in the runner's git config during the workflow execution.Source: Linters/SAST tools
🤖 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 152-160: The download-artifact action's path parameter is set to
"gas.txt", which creates a subdirectory and places the downloaded file at
"gas.txt/gas.txt". However, the add-pr-comment action's message-path parameter
expects the file to be at the root. Change the path parameter in the
download-artifact action from "gas.txt" to "." (current directory) so that the
gas-report artifact is extracted directly to the root where message-path can
locate it.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 19-20: Add `persist-credentials: false` parameter to all checkout
steps in the GitHub Actions workflow for supply-chain security hardening. In
`.github/workflows/ci.yml` at lines 19-20 (yarn-install job checkout), lines
45-46 (test job checkout), lines 80-81 (size job checkout), and lines 108-109
(coverage job checkout), add the `persist-credentials: false` option to each
`actions/checkout` step to prevent git credentials from persisting in the
runner's git config and reduce attack surface if dependencies or subsequent
steps are compromised.
- Around line 14-39: The checkout action step at the "Checkout repository" task
is not explicitly disabling git credential persistence, which leaves the runner
vulnerable to credential exfiltration from compromised dependencies. Add a with
section to the checkout step and include persist-credentials: false as a
configuration option to explicitly prevent git credentials from being persisted
in the runner's git config during the workflow execution.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: db3974de-c35f-4b77-b598-fa5ca1e74e0d
📒 Files selected for processing (2)
.github/workflows/ci.yml.yarnrc
Signed-off-by: Cal Mac Fadden <108666242+calmacfadden@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
68-71:⚠️ Potential issue | 🟠 MajorPin Foundry to a specific nightly commit hash across all jobs.
The
foundry-rs/foundry-toolchainaction is SHA-pinned, butversion: nightlystill downloads a moving Foundry binary. Useversion: nightly-<commit-hash>instead to ensure reproducible, deterministic CI runs. This prevents test, size, and coverage behavior from changing unexpectedly across runs.Apply to lines 68-71, 103-106, and 131-134 in
.github/workflows/ci.yml:version: nightly-<specific-commit-hash>🤖 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 68 - 71, The version setting for the Foundry toolchain is set to `version: nightly`, which downloads a moving binary instead of pinning to a specific commit, causing non-deterministic CI behavior. In the .github/workflows/ci.yml file, locate all instances where the `foundry-rs/foundry-toolchain` action has `version: nightly` (appearing in the "Install Foundry" step at lines 68-71, 103-106, and 131-134) and replace `version: nightly` with `version: nightly-<specific-commit-hash>` where `<specific-commit-hash>` is a pinned nightly commit hash to ensure reproducible and deterministic builds across all jobs.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
45-56: Validate the effectivenodeLinkerin addition to its YAML declaration.The step greps for
nodeLinker: node-modulesin the static file but doesn't assert the effective Yarn config fornodeLinker. A duplicate key or environment override could pass the grep while Corepack installs with a different linker. The effectiveenableScriptsandnpmMinimalAgeGateare validated viayarn config get, butnodeLinkeris missing from this validation.Suggested change
enable_scripts="$(corepack yarn config get enableScripts)" + node_linker="$(corepack yarn config get nodeLinker)" minimal_age_gate="$(corepack yarn config get npmMinimalAgeGate)" - echo "enableScripts=${enable_scripts} npmMinimalAgeGate=${minimal_age_gate}" + echo "enableScripts=${enable_scripts} nodeLinker=${node_linker} npmMinimalAgeGate=${minimal_age_gate}" test "${enable_scripts}" = "false" + test "${node_linker}" = "node-modules" test "${minimal_age_gate}" = "10080"🤖 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 - 56, The workflow is validating the effective configuration for enableScripts and npmMinimalAgeGate using `corepack yarn config get` commands, but nodeLinker is only checked in the static .yarnrc.yml file via grep. To ensure the effective nodeLinker configuration is actually "node-modules" (and not overridden elsewhere), add a new command that captures the effective nodeLinker value using `corepack yarn config get nodeLinker`, store it in a variable similar to enable_scripts and minimal_age_gate, include it in the echo statement for visibility, and add a test assertion to verify it equals "node-modules".
🤖 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 22-23: Add `persist-credentials: false` to all four checkout steps
in the workflow file to enforce least privilege and prevent unnecessary
persistence of the job token. In the checkout steps at lines 22-23 (yarn-install
job), 65-66 (test job), 100-101 (size job), and 128-129 (coverage job), add the
`persist-credentials: false` parameter to each `actions/checkout` action
configuration. This disables credential persistence since none of these jobs
perform authenticated Git operations requiring stored credentials.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 68-71: The version setting for the Foundry toolchain is set to
`version: nightly`, which downloads a moving binary instead of pinning to a
specific commit, causing non-deterministic CI behavior. In the
.github/workflows/ci.yml file, locate all instances where the
`foundry-rs/foundry-toolchain` action has `version: nightly` (appearing in the
"Install Foundry" step at lines 68-71, 103-106, and 131-134) and replace
`version: nightly` with `version: nightly-<specific-commit-hash>` where
`<specific-commit-hash>` is a pinned nightly commit hash to ensure reproducible
and deterministic builds across all jobs.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 45-56: The workflow is validating the effective configuration for
enableScripts and npmMinimalAgeGate using `corepack yarn config get` commands,
but nodeLinker is only checked in the static .yarnrc.yml file via grep. To
ensure the effective nodeLinker configuration is actually "node-modules" (and
not overridden elsewhere), add a new command that captures the effective
nodeLinker value using `corepack yarn config get nodeLinker`, store it in a
variable similar to enable_scripts and minimal_age_gate, include it in the echo
statement for visibility, and add a test assertion to verify it equals
"node-modules".
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5be75610-ddcd-46d9-814e-4a7a86b08309
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
.github/workflows/ci.yml.yarnrc.ymlpackage.json
✅ Files skipped from review due to trivial changes (2)
- package.json
- .yarnrc.yml
Summary by CodeRabbit
Chores