[codex] Centralize libyaml setup in Ruby action#3758
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCentralize Ruby/Bundler setup into a repo-local composite action: extend ChangesRuby/Bundler setup centralization
Sequence Diagram(s)sequenceDiagram
participant WorkflowJob
participant setup_bundle as ./.github/actions/setup-bundle
participant setup_ruby as ./.github/actions/setup-ruby
participant upstream as ruby/setup-ruby@v1
WorkflowJob->>setup_bundle: call setup-bundle (with install-libyaml)
setup_bundle->>setup_ruby: delegate Ruby/Bundler setup (bundler-version, frozen, install-libyaml, bundler-cache)
setup_ruby->>upstream: invoke ruby/setup-ruby@v1 (cached or non-cached path)
upstream-->>setup_ruby: install Ruby / manage Bundler cache
setup_ruby-->>setup_bundle: return status
setup_bundle-->>WorkflowJob: continue job steps
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Code Review Summary: Clean CI refactor that centralizes libyaml-dev installation and Ruby/Bundler setup into a single composite action. The approach is sound and the implementation is correct. What is Good
Minor Observations
Verdict No blocking issues. Centralization is correct, the libyaml conditional is properly guarded by OS and input flag, and setup-bundle delegation preserves original BUNDLE_FROZEN/RETRY/JOBS semantics. Recommend: approve once CI is green. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
justin808
force-pushed
the
codex/issue-3521-centralize-libyaml
branch
from
e29b913 to
6e91005
Compare
Code Review: Centralize libyaml setup in Ruby actionSummary: Clean centralization that moves libyaml installation and the conditional bundler-cache path into the shared CorrectnessBenchmark workflow — both changes are correct:
String comparisons —
Existing workflows will pay a redundant libyaml installSeveral workflows call both These workflows can be simplified in a follow-up: drop the standalone Minor API design note
NitIn the non-cache path (first Overall: Approved. The centralization is correct, the benchmark workflow changes are clean, and the bundler-cache branching keeps |
justin808
force-pushed
the
codex/issue-3521-centralize-libyaml
branch
from
6e91005 to
d771225
Compare
Greptile SummaryThis PR centralizes the
Confidence Score: 4/5Safe to merge; changes are CI composite-action wiring only with no runtime code impact. The refactoring correctly centralizes libyaml setup and Bundler cache wiring. The benchmark job ends up running apt-get install libyaml-dev twice per run (once via the standalone setup-ruby call, once via setup-bundle to setup-ruby), which the PR itself flags as a known follow-up. No functional breakage was found. benchmark.yml — the benchmark job calls setup-ruby standalone then setup-bundle (which calls setup-ruby again), resulting in a double libyaml-dev install per run. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[benchmark.yml / workflow] -->|plain Ruby setup| B[setup-ruby action]
A -->|gems needed| C[setup-bundle action]
B --> D{Linux and install-libyaml?}
D -->|yes| E[apt-get install libyaml-dev]
D -->|no| F{bundler-cache == true?}
E --> F
F -->|no| G[ruby/setup-ruby v1 no cache]
F -->|yes| H[ruby/setup-ruby v1 with cache and BUNDLE env]
C --> I[Verify Gemfile.lock committed]
I --> J[setup-ruby action bundler-cache true]
J --> K{Linux and install-libyaml?}
K -->|yes| L[apt-get install libyaml-dev again]
K -->|no| M[ruby/setup-ruby v1 with cache]
L --> M
style L fill:#ffc,stroke:#aa0
Reviews (1): Last reviewed commit: "ci: centralize libyaml setup in ruby act..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Code ReviewOverviewClean, well-structured centralization. The two-step split to work around GitHub Actions' lack of conditional IssuesDouble
|
justin808
force-pushed
the
codex/issue-3521-centralize-libyaml
branch
2 times, most recently
from
bea8f02 to
4741434
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@analysis/vm-script-caching-investigation-2026-06-07.md`:
- Around line 162-165: The artifact pointer "Original experiment:
`vm-script-cache/tmp/experiments/20260607T083551Z-vm-script-cache/`" is an
ephemeral local tmp path; update the pointer to a durable location by replacing
that tmp path with a stable repo artifact or permalink (e.g., a committed repo
path, an artifacts/ directory, or a permalink to the experiment bundle) or
include a commit SHA/CI artifact URL and attach the raw data so future readers
can verify the experiment; ensure the updated reference is placed where
"Original experiment" is declared.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f345143-e7e0-4498-81b4-4ea718d77d75
📒 Files selected for processing (4)
.github/actions/setup-bundle/action.yml.github/actions/setup-ruby/action.yml.github/workflows/benchmark.ymlanalysis/vm-script-caching-investigation-2026-06-07.md
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/actions/setup-ruby/action.yml
- .github/actions/setup-bundle/action.yml
- .github/workflows/benchmark.yml
|
Code Review -- Overview: This PR centralizes libyaml-dev installation into the shared setup-ruby composite action. setup-bundle now delegates to setup-ruby instead of calling ruby/setup-ruby@v1 directly, and benchmark.yml drops its manual apt-get steps. The analysis doc changes are pure Prettier formatting. Overall a solid cleanup; a few items worth addressing before merge. Issues: (1) Double libyaml install per run -- In the benchmark job, both the standalone setup-ruby step (line 245) and the downstream setup-bundle step (line 330, which internally calls setup-ruby) will run apt-get update && apt-get install libyaml-dev since install-libyaml defaults to true. The same pattern repeats in every workflow with standalone setup-ruby followed by setup-bundle in the same job (examples.yml, lint-js-and-ruby.yml, gem-tests.yml, several pro- workflows). It is idempotent but costs 5-15s per job. Recommended fix: expose install-libyaml as a passthrough input on setup-bundle (default true) and forward it to the internal setup-ruby call so callers that already ran setup-ruby can pass install-libyaml: false to setup-bundle to skip the duplicate. (2) Code contradicts PR description on bundler-cache: false -- The NON_BLOCKING_DECISION note in the PR body says keep bundler-cache: false explicit in the non-cache setup path for readability, but the non-cache Setup Ruby step in setup-ruby/action.yml does not pass it. Relying on the upstream default is fine functionally; either add the parameter or update the description to match. (3) BUNDLE_RETRY and BUNDLE_JOBS hardcoded in shared action (minor) -- These were hardcoded in setup-bundle before and are now hardcoded in setup-ruby. No regression, but promoting them to the shared layer makes them non-overridable without a flag. Consider exposing as optional inputs. Not a blocker. What is good: install-libyaml defaults to true so the action is safe-by-default. The two-step cache/non-cache split with the explaining comment is the right pattern for GitHub Actions lack of conditional env blocks. The setup-bundle description update correctly reflects that libyaml is now handled internally. The bundler to bundler-version rename on setup-ruby public interface is consistent. |
justin808
force-pushed
the
codex/issue-3521-centralize-libyaml
branch
from
4741434 to
1404407
Compare
justin808
dismissed
coderabbitai[bot]’s stale review
Dismissed stale CodeRabbit requested-changes review from an older commit. The actionable markdown artifact-path comment was fixed in current head 1404407, and the CodeRabbit status check is now green. Merge gating is CI plus addressed substantive review comments.
|
Code Review Summary What is good: Correct workaround for GH Actions conditional env limitation; clean bundler-version rename; backward-compatible; thorough validation in PR description. Issues found:
Verdict: Changes are correct and safe - no blocking issues. Item 1 is the most impactful and should be tracked promptly. |
|
Code Review: Centralize libyaml setup in Ruby action. Overall: Clean, low-risk CI infra refactor - see inline comments for details. |
|
Latest review-thread triage for
|
|
Code Review — Centralize libyaml setup in Ruby action Overall assessment: Clean, low-risk CI infrastructure refactor. Centralization is well-structured, actionlint / Prettier / YAML parse validation is thorough, and the behavioral change is small and idempotent. What the PR does
Issues / discussion points 1. Double libyaml install in workflows that pair setup-ruby + setup-bundle (acknowledged as FOLLOWUP) About 15 jobs across 8 workflows call setup-ruby (Ruby runtime only) and then setup-bundle (which now calls setup-ruby again with bundler-cache: true). Both calls default to install-libyaml: true, so apt-get update && apt-get install -y libyaml-dev runs twice per job. Functionally safe since it is idempotent, but adds ~3-5 s overhead per run. A short-term fix would be to expose install-libyaml as a passthrough input in setup-bundle so callers can skip it when setup-ruby already ran; the longer-term fix is the one noted in the PR: remove the standalone setup-ruby call from jobs where setup-bundle should own the full setup. 2. apt-get update added to benchmark paths that previously skipped it The old benchmark steps ran sudo apt-get install -y libyaml-dev (no update). The new setup-ruby action runs sudo apt-get update && apt-get install -y --no-install-recommends libyaml-dev. The --no-install-recommends flag and the explicit update are both improvements for cold-runner correctness, but add a few extra seconds to benchmark jobs on every run. Acceptable tradeoff, just worth noting. 3. BUNDLE_FROZEN with string false in non-frozen matrix legs — works correctly Workflows like dummy-app-bundler-tests.yml pass frozen via a ternary that can produce the string false. The env var BUNDLE_FROZEN=false is set. Bundler correctly treats this as boolean false (matches the false case in Bundler's to_bool helper), so minimum-dependency legs install without frozen mode as intended. No bug, but worth a comment in the action for future readers. 4. install-libyaml not passable from setup-bundle setup-bundle does not forward an install-libyaml option to setup-ruby, so callers cannot opt out when libyaml is already installed. Not a problem today (item 1 shows it is just idempotent overhead), but it removes flexibility for non-Linux or pre-warmed runners. Low priority given the current runner profile. Minor nit (not blocking) The setup-bundle description now reads Setup Ruby, cache, and install gems for a Bundler project. Since setup-bundle now handles libyaml automatically, a note like includes libyaml-dev on Linux runners would keep it self-documenting. The old description warned callers to install it themselves; the new one gives no hint either way. Summary The refactor achieves its goal: one place owns libyaml + Ruby setup, direct ruby/setup-ruby@v1 usage is gone from all workflows/actions except inside the canonical action, and the benchmark workflow is simplified. The double-install consequence is the main operational concern and is correctly flagged as FOLLOWUP. |
Code Review: Centralize libyaml setup in Ruby actionSummary: This PR cleanly centralizes libyaml-dev installation and the Bundler-cache wiring into the shared Positives
Issues / Suggestions
The description reads: description: Whether to install libyaml-dev before Ruby/Bundler steps.But the implementation guards on description: >
Whether to install libyaml-dev before Ruby/Bundler steps.
Only takes effect on Linux runners; silently ignored elsewhere.Hardcoded The Bundler env vars Two-step split creates parallel maintenance surface The split into two nearly-identical Correctness check:
|
Code Review - see full review below |
justin808
force-pushed
the
codex/issue-3521-centralize-libyaml
branch
from
95cc16c to
506ac6a
Compare
|
test discard |
Code ReviewOverviewThis PR centralizes What is Working Well
Issues / Suggestions1. Footgun potential: default Both
2. The libyaml step always runs run: >
sudo apt-get install -y --no-install-recommends libyaml-dev 2>/dev/null ||
(sudo apt-get update && sudo apt-get install -y --no-install-recommends libyaml-dev)This pattern pre-existed this PR, so purely advisory for a follow-up issue. 3. Explicit The non-cache VerdictLogic is sound, all call sites are consistently updated, and the validation steps were comprehensive. Issues 1 and 2 are non-blocking advisory suggestions. Ready to merge once CI is green. |
Code Review: Centralize libyaml setup in Ruby actionOverviewThis PR cleanly centralizes What Works WellCorrect call-site pattern across all workflows. Every job that calls both Elegant workaround for GitHub Actions' conditional env limitation. The two-step split in
Bundler env vars correctly scoped. Bundler version defaults preserved. Minor Observations1. String equality for if: inputs.bundler-cache != 'true' # non-cache path
if: inputs.bundler-cache == 'true' # cache pathGitHub Actions inputs are always strings, so this is correct. However, it means only the literal string 2. Opt-out pattern requires discipline from future contributors The default 3. run: sudo apt-get update && sudo apt-get install -y --no-install-recommends libyaml-devThis is the correct and safe approach for GitHub-hosted runners (stale apt indices are a known source of failures). Confirming it is intentional — no action needed. Analysis Document ChangesThe reformatting of VerdictNo blocking issues. The centralization logic is sound, consistently applied, and well-validated (actionlint, YAML parse check, Prettier). Ready to merge once CI is green. |
justin808
force-pushed
the
codex/issue-3521-centralize-libyaml
branch
from
506ac6a to
fbd2dd0
Compare
|
Code Review Summary: Clean centralization of libyaml-dev installation. Moves install responsibility into setup-ruby, removes ad-hoc apt-get steps from benchmark.yml, and wires an install-libyaml passthrough through the action chain. YAML-parser validation and actionlint pass confirm the mechanics are solid. Issues Minor - opt-out footgun in setup-bundle install-libyaml defaults to 'true' in setup-bundle. The contract is: any caller that already ran setup-ruby in the same job must remember to pass install-libyaml: 'false' to setup-bundle, or libyaml gets installed twice (harmless, but wastes ~20 s per job). This PR handles all 13 existing call sites correctly, but future contributors adding a new job following the setup-ruby + setup-bundle pattern will silently double-install unless they know the rule. Suggestion: extend the install-libyaml description in setup-bundle/action.yml to say: "Pass 'false' if a prior setup-ruby step in this job already installed libyaml-dev." Observation - double Ruby setup in the benchmark job (pre-existing) In the benchmark job, setup-ruby at line 246 sets up the Ruby runtime for gem install foreman, then setup-bundle at line 330 calls setup-ruby again (with bundler-cache: true) to install app gems. ruby/setup-ruby is idempotent so this is harmless, but it is redundant. Pre-existing pattern, worth a future cleanup. Positive highlights
Verdict: Approve with the description-improvement suggestion above. The structural change is sound, validation is comprehensive, and the risk is low. |
Code Review — PR #3758: Centralize libyaml setup in Ruby actionOverviewThis PR centralizes Scope: 13 files, all What's Working WellCorrect two-step split in Consistent
Better
Observations
Minor Suggestions
SummaryThe centralization is sound. The logic is correct throughout, the two-step workaround is properly handled, and the Verdict: Looks good to merge once CI is green. |
Code Review — #3758 Centralize libyaml setup in Ruby actionOverviewClean, well-scoped refactoring. Centralizing Correctness
Improvements vs. prior code
Pre-existing concern (not introduced here, but now more impactful)
Minor / optional
Verdict: Ready to merge once CI is green. No blocking issues found. |
…orkflow-build-config-scope-nonforce * origin/main: [codex] Centralize libyaml setup in Ruby action (#3758)
…un-skipped-ci-retry * origin/main: [codex] Centralize libyaml setup in Ruby action (#3758)
1 participant
Closes #3521.
Summary
setup-rubycomposite action so it can optionally runruby/setup-rubywith Bundler cache settings.setup-bundledelegate Ruby setup and gem caching through the sharedsetup-rubyaction instead of callingruby/setup-rubydirectly.libyaml-devinstall steps with the sharedsetup-rubyaction.install-libyamlpassthrough so workflows that already ransetup-rubycan callsetup-bundlewithout a duplicatelibyaml-devinstall.install-libyamlonly takes effect on Linux runners, matching the guarded install step.Validation
actionlint- passed.ruby -e 'require "yaml"; files = Dir[".github/actions/*/action.yml", ".github/workflows/*.yml"].sort; files.each { |path| YAML.load_file(path) }; puts "parsed #{files.size} yaml files"'- parsed 30 YAML files.pnpm dlx prettier@3.6.2 --check .github/actions/setup-ruby/action.yml .github/actions/setup-bundle/action.yml .github/workflows/benchmark.yml .github/workflows/dummy-app-bundler-tests.yml .github/workflows/examples.yml .github/workflows/gem-tests.yml .github/workflows/lint-js-and-ruby.yml .github/workflows/playwright.yml .github/workflows/precompile-check.yml .github/workflows/pro-integration-tests.yml .github/workflows/pro-test-package-and-gem.yml .github/workflows/shakaperf-release-gates.yml analysis/vm-script-caching-investigation-2026-06-07.md- passed.git diff --check- passed.script/ci-changes-detector origin/main- CI infrastructure; recommended full suite, no benchmarks.rg -n "sudo apt-get .*libyaml|Install libyaml-dev|ensure libyaml-dev|ruby/setup-ruby@v1|install-libyaml" .github/workflows .github/actions- directlibyaml-devinstall remains centralized insetup-ruby; workflowsetup-bundlecalls that already ransetup-rubynow passinstall-libyaml: 'false'.Adversarial PR Review Gate
maincodex/issue-3521-centralize-libyamldfbbeac38b82e54b51e51f7f739a4d2fbf8d24c9rgover.github/actionsand.github/workflows, YAML parser check,actionlint, Prettier check,script/ci-changes-detector origin/main,git diff --check.Findings:
BLOCKING: none remaining after current-head review fixes.DISCUSS: none requiring maintainer input.FOLLOWUP: optional future optimization: try installinglibyaml-devbeforeapt-get updateon warm runners. This is advisory, pre-existing behavior, and outside the centralization scope of this PR.NON_BLOCKING_DECISION: keepruby/setup-ruby@v1usage inside the sharedsetup-rubyaction; the centralization target is removal of workflow/local-action bypasses and manuallibyaml-devinstalls, not vendoring the upstream Ruby setup action. Keepbundler-cache: falseexplicit in the non-cache setup path for readability, even though it matches the upstream default. Keepsetup-bundlefocused on bundle setup while documenting the Linux-onlyinstall-libyamlbehavior insetup-ruby; workflow call sites already passinstall-libyaml: 'false'whensetup-rubyhas already run. Keep thesetup-rubysplit comment concise: the cache path is the only path where Bundler env vars can matter because the non-cache path does not run bundle install.NOISE: no code-runtime validation was run because this is CI composite-action wiring; GitHub Actions checks are the authoritative integration validation.Readiness note: non-draft and ready to merge once current-head CI is green.
Note
Low Risk
CI composite-action wiring only; behavior should match prior Ruby/Bundler and libyaml steps, with integration validated by GitHub Actions runs.
Overview
libyaml and Ruby/Bundler setup are consolidated in the local
setup-rubycomposite action, andsetup-bundlenow delegates to it instead of callingruby/setup-rubydirectly.setup-rubygains optional Bundler cache install (working-directory,bundler-cache,frozen,cache-version) with separate steps for cache vs non-cache paths, and keepslibyaml-devinstall behindinstall-libyaml(Linux only).setup-bundleadds aninstall-libyamlpassthrough; workflows that already runsetup-rubypassinstall-libyaml: 'false'on subsequentsetup-bundlesteps to avoid duplicate apt installs. Benchmark workflow drops inlinelibyaml-devsteps and uses./.github/actions/setup-ruby.The VM script-caching analysis note is updated so artifact references stay in the committed doc instead of an ephemeral tmp path.
Reviewed by Cursor Bugbot for commit dfbbeac. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit