fix(ci): skip LFS hydration when no files are tracked - #6141
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used🪛 zizmor (1.29.0).github/workflows/ci-builds.yml[warning] 250-299: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false (artipacked) [warning] 1-915: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block (excessive-permissions) [warning] 103-915: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block (excessive-permissions) 🔇 Additional comments (2)
📝 WalkthroughWalkthroughThe CI builds workflow now hydrates vendored source archives only for trusted inputs with LFS-tracked files. It avoids the unconditional ChangesCI LFS hydration
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🟡 Moderate · up to This workflow now avoids failing Git LFS hydration when no files are tracked and restricts hydration and source verification to trusted builds. However, the deployed fork-CI caller still uses an older workflow revision, so fork jobs do not receive the new security gate and may continue running repository-provided verification logic; update the caller pin or explicitly accept this gap before relying on the fix. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01b01c8302
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # test is what actually skips it. | ||
| - name: Hydrate vendored source archives | ||
| if: ${{ steps.cache-check.outputs.cache-hit != 'true' }} | ||
| if: ${{ inputs.trusted && steps.cache-check.outputs.cache-hit != 'true' }} |
There was a problem hiding this comment.
When inputs.trusted is false and the target ref contains LFS-managed dependencies, this condition skips hydration even though the later Build step at line 329 is not gated on inputs.trusted and therefore still runs. Because checkout sets GIT_LFS_SKIP_SMUDGE=1, that build receives pointer stubs instead of the vendored archives and fails. Either keep this non-privileged hydration step enabled for untrusted runs or also prevent the build from running in that mode.
Useful? React with 👍 / 👎.
|
1 participant
Problem
PR #6135 changed CI-builds to hydrate LFS archives generically. On branches with no
.gitattributesLFS entries, including v3.0,git lfs pullexits with status 1. All CI-builds matrix legs consequently fail before the build begins.The hydration step also executed its branch-provided verifier loop for untrusted fork calls, despite the reusable workflow contract stating that only checkout runs in that mode.
Fix
git lfs ls-files --name-onlyto detect whether the checked-out branch tracks LFS files, and callgit lfs pullonly when it does.inputs.trusted, consistent with the fork-CI security model.Verification
ruby -e 'require "yaml"; YAML.load_file(".github/workflows/ci-builds.yml")'actionlint .github/workflows/ci-builds.ymlLocal
git-lfsis unavailable in this checkout; CI will exercise both the no-LFS v3.0 path and LFS-enabled branches.Summary by cubic
Fixes CI builds failing on branches with no LFS-tracked files.
git lfs pullpreviously ran unconditionally and exited 1 on branches without.gitattributesLFS entries, breaking every build before it started.Written for commit 01b01c8. Summary will update on new commits.
Summary by CodeRabbit