Fix ci-required base ref for PR merge-ref checkouts

[justin808]

Fixes #4756

Correcting the record first

#4756 is titled and framed around a "shallow merge-base fallback": fetch-depth: 50 too small, merge base unresolvable, fallback widens the diff. Fetch depth was never the cause. I reproduced the bug on a full, unshallow clone, which by itself rules depth out.

Three pieces of evidence from the run the issue cites:

1. The failing run checked out refs/remotes/pull/4739/merge = ed86829364509ac3e4e3782e52f44300d11e0f17. So HEAD was GitHub's PR merge commit, not the PR head. Its parents are 613c6c2a (base side) and bf4711c1 (PR head side).
2. The workflow passed github.event.pull_request.base.sha = 18bdc4f8, dated 2026-07-18T08:06:09Z. The run was at 23:33Z, so the base SHA was about 15 hours behind the merge commit's own base parent. merge-base(18bdc4f8, HEAD) = 18bdc4f8 is the correct answer for those inputs. The inputs were wrong, not the merge-base resolution.
3. The log contains zero Deepening shallow history lines. The deepen loop never ran. script/lib/git-diff-base already deepens 50 → 100 → ... → --unshallow and hard-fails with an explicit diagnostic when a base is genuinely unreachable, so the "deepen or fail closed" half of the request is already implemented upstream.

The clincher: the post-rebase passing run used base 613c6c2a, which is exactly HEAD^1 of the failing run's merge commit. Had the failing run used HEAD^1, it would have classified documentation-only with no rebase and no hosted CI.

Root cause

For pull_request events actions/checkout resolves refs/pull/<n>/merge, so HEAD is the PR head merged into the current base tip. github.event.pull_request.base.sha is a different commit: it only refreshes when the PR is opened or synchronized. On a branch that has not been pushed for a while the two drift apart, and diffing from the stale SHA folds every base-branch commit in between into the changed-file set. A one-file docs-only PR then looks like it touched generators.

The fix

Take the base from the merge commit's first parent, which is the exact commit the merge was computed against, guarded on HEAD having exactly two parents.

.github/workflows/bundle-size.yml:82-99 already derives its size baseline from the same first parent, with the same documented reasoning. This follows that precedent rather than inventing a mechanism.

fetch-depth stays at 50. The first parent is a parent of HEAD, so it can never fall outside the shallow window, and widening the fetch would slow every run for no benefit.

Reproduction

A docs-only branch, a simulated refs/pull/N/merge (first parent = main tip, second parent = PR head), and a stale base.sha standing in for the 15-hour lag. Run on a full clone (git rev-parse --is-shallow-repository = false).

HEAD (merge ref)  : 0e95f13df4cbfe18b77e5c596ca734efaf51832e
HEAD^1 (base side): 822b4c30bc3764a08454a7e891834b2699921a03
HEAD^2 (PR head)  : 6299aaeb2045a6c0347837528a841351becece9e
stale base.sha    : 1c12dbbb91eeb97cf006ce39c02621e98502a98c
PR touches        : internal/planning/REPRO_4756.md

BEFORE: base = stale pull_request.base.sha

Base: 1c12dbbb91eeb97cf006ce39c02621e98502a98c | Current: HEAD | Merge base: 1c12dbbb91eeb97cf006ce39c02621e98502a98c

Changed file categories:
  • JavaScript/TypeScript code
  • RSpec tests
  • Generators
  • React on Rails Pro Ruby core source code
  • React on Rails Pro Dummy app
  • CI infrastructure (full test suite, no benchmarks)
  • Uncategorized changes (running full suite for safety)

---- GITHUB_OUTPUT flags:
docs_only=false
non_runtime_only=false
run_js_tests=true
run_generators=true

That run_generators=true is what fails required-pr-gate and tells the author to spend a hosted CI run.

AFTER: same stale base.sha in the environment, fix applied

This is not a hand-written recreation. The step's run: body was extracted straight out of the committed .github/workflows/ci-required.yml with a YAML parser and executed, so what is tested is exactly what ships.

Diff base: 822b4c30bc3764a08454a7e891834b2699921a03 (source: PR merge commit first parent)
=== CI Changes Analysis ===
Base: 822b4c30bc3764a08454a7e891834b2699921a03 | Current: HEAD | Merge base: 822b4c30bc3764a08454a7e891834b2699921a03

✓ Documentation-only changes

Recommended CI jobs: NONE (skip CI)
Changed files the classification was computed from (first 50):
internal/planning/REPRO_4756.md

---- GITHUB_OUTPUT flags:
docs_only=true
non_runtime_only=true
run_js_tests=false
run_generators=false

Branch matrix

This is now a committed test rather than a one-off: script/ci-required-diff-base-test.bash, wired into this workflow's own Run CI gate tests step. It extracts the step body from the committed YAML with a YAML parser and runs it against a synthetic repository under bash --noprofile --norc -eo pipefail, the exact shell Actions uses.

-> current merge ref uses the first parent
-> stale merge ref fails closed
-> single-parent HEAD keeps the event base
-> workflow_dispatch input wins
-> merge_group keeps the event base
-> large changed-file list does not abort the step

6 diff-base tests passed

Two cases carry most of the weight. A single-parent HEAD must never take HEAD^1, because there it is the PR's own previous commit rather than the base branch. And a stale merge ref must not use its first parent at all, because that base describes an older head and would narrow the diff (decision log item 6).

The harness is a real guard, not a rubber stamp. Reverting each fix makes it fail:

# with `git diff --name-only | head -50` restored
FAIL: large changed-file list: expected exit 0, got 141
2 of 6 diff-base tests failed

# with the stale-merge-ref guard removed
FAIL: stale merge ref: expected exit 1, got 0.
      Output: Diff base: 268edd42... (source: PR merge commit first parent)
FAIL: stale merge ref: did not expect 'DETECTOR_BASE=' in: ...

141 is the SIGPIPE exit the reviewers predicted.

The stale-merge-ref case also asserts directly that the newer head's file is absent from the widest possible diff against the stale HEAD. That assertion is the evidence for decision log item 6: it is why no choice of base could have fixed this, and why the step refuses instead.

The fix is not total, by design

There is one fallback path that lands on EVENT_BASE_REF — the stale github.event.pull_request.base.sha. On that path the original over-selection can still occur, so a stale docs-only PR in that state still gets a wide classification.

That is deliberate. A stale event base can only widen the diff, which wastes runner minutes; it cannot let an unverified change through. The second abnormal state does not fall back at all — it fails the gate, because widening the base cannot repair a HEAD that is missing the current head's changes. Neither state is silent: the fallback path emits a ::warning:: annotation plus the printed changed-file list, and the fail-closed path emits ::error:: and exits non-zero.

• Single-parent HEAD (GitHub produced no merge ref, in practice a PR conflicting with its base): no better base exists, so warn and continue. Decision log item 1.
• Stale merge ref (second parent is not the current PR head): does not fall back — it fails the gate. HEAD's tree is missing the current head's changes, so no base can produce a trustworthy classification. Decision log item 6.

Scope

Two files: .github/workflows/ci-required.yml and the new script/ci-required-diff-base-test.bash.

Nine other hosted workflows feed the detector the same stale base.sha and have the same latent misrouting — tracked in #4818. Moving this step's logic out of YAML into script/ is tracked in #4824. Neither is widened into this PR.

Post-merge exercise

Required by .github/read-me.md for semantic .github/workflows/** changes: #4820.

The stale-base path cannot be exercised from this branch, because a freshly pushed PR always has a current base.sha, so this PR's own run only covers the case where old and new logic agree. #4820 specifies the throwaway stale docs-only PR, the expected step-log evidence, and the cleanup.

Changelog

No changelog entry applies. CHANGELOG.md is user-visible changes only per the repo seam in .agents/agent-workflow.yml. This is internal CI plumbing with no effect on the published gem or npm package.

Labels

Labels: ci-tooling because the change is confined to CI workflow suite-routing logic and ships no runtime code.

Codex Decision Log

1. Warn instead of hard-fail when HEAD is not a two-parent merge commit. bundle-size.yml hard-fails with ::error:: in the equivalent situation; this deliberately diverges. GitHub does not produce a merge ref for a PR that conflicts with its base, so a single-parent HEAD on a pull_request event is a real, reachable state rather than a can't-happen. ci-required is the required gate on every PR, so hard-failing there would block the gate on every conflicted PR, which is strictly worse than the misclassification it guards against, and a conflicted PR cannot merge anyway. bundle-size.yml can afford ::error:: precisely because it is advisory and this gate is not. Please do not "fix" this back to a hard fail without that asymmetry in mind.

2. fetch-depth left at 50. The issue suggested deepening. The first parent is a parent of HEAD and therefore always in the clone, so deepening buys nothing and would slow every PR.

3. script/ci-changes-detector untouched. The issue suggested changing the detector. The detector's merge-base resolution is correct; only its input was wrong. Fixing the input keeps the blast radius to one file.

4. Stale merge ref warns but still uses the first parent. Reversed in review — this was wrong. See item 6.

5. internal/planning/CONTEXT.md not modified, although it was in scope for this lane. That file is a product-strategy glossary (Language / Relationships / Example dialogue / Flagged ambiguities) and documents no CI decisions, so adding one would have meant inventing a new section style in a tightly scoped document. CI routing behavior is documented in .github/read-me.md and decisions in internal/adr/, neither of which was in scope. The decision record lives in this PR body and in the inline comments on the changed step.

6. A stale merge ref fails closed. This is the subtle one, and it took two corrections to get right — read the direction of the error, and then read which half of the diff it applies to.

   First correction (from Greptile): my original code only warned and kept classifying from the stale merge ref's first parent. Compare which way each base can be wrong:

   • A stale event base can only add base-branch commits to the diff. Over-selects suites, wastes runner minutes. Safe direction, and why CI: nine hosted workflows feed the changed-files detector a stale pull_request.base.sha #4818 is a cost problem rather than a correctness hole.
   • A stale merge ref's first parent describes an older head, so changes in the current head get omitted. run_generators can come back false and this required gate can pass without the hosted run those changes needed. Unsafe direction.

   So I switched to falling back to EVENT_BASE_REF. That was still wrong, and this is the part a future reader will most likely get wrong too, because I did and so did the reviewer who proposed it.

   Second correction (from Codex): widening the base does not fix under-selection, because the base is only half of the diff. The detector diffs the chosen base against HEAD. When the merge ref is stale, HEAD's tree does not contain the current head's changes at all. Changing the base changes what we diff from; it does nothing about what we diff to. A file added in the newer head stays invisible no matter how wide the base is. There is no base that repairs a tree which is already missing the changes.

   That leaves two honest options: fetch and classify the reported head (a network fetch, and then the classified tree no longer matches what the rest of the job checked out), or refuse to classify. A required gate should refuse. The step now exits 1 with a message saying to re-run so GitHub recomputes refs/pull/<n>/merge, or push again — the condition is a race against that recomputation, so a re-run clears it.

   A third option, emitting a maximal conservative classification instead of failing, was rejected on evidence rather than taste. It does not avoid a red gate, and it makes the failure misleading:

   $ RUN_GENERATORS=true SHOULD_RUN_HOSTED_CI=false bash script/ci-required-hosted-gate
   Generator changes require hosted CI before merge.
   Comment +ci-run-hosted on the PR, or add the ready-for-hosted-ci label...
   EXIT: 1
   

   That is the exact wrong-remedy message ci-required: shallow merge-base fallback misclassifies stale docs-only PRs as generator changes #4756 exists to eliminate, and it would route someone to spend a full hosted run over a transient merge-ref race. Same availability cost, worse diagnosis.

   Note the asymmetry with item 1, which still stands: on a single-parent HEAD we warn and continue, because HEAD is the real PR head there and a wide base still yields a complete diff. Positive proof that the checkout is wrong is what separates the two cases.

7. The changed-file list is captured before truncation, and truncated with sed rather than head. Two reviewers independently found that git diff --name-only | head -50 breaks under the Actions shell (bash --noprofile --norc -eo pipefail): head closes the read end, git takes SIGPIPE and exits 141, pipefail promotes it, set -e fails the gate. It would fire on large diffs — the widened-diff case this output exists to expose — so a cosmetic diagnostic could have taken down the required gate exactly when it had something to show. sed -n '1,50p' reads its input to the end, so nothing closes early. mapfile with an array slice reads better but needs bash 4+, and this repo's *-test.bash harnesses run on macOS where /bin/bash is 3.2.

8. The changed-file assignment is guarded — for the diagnostic, not for control flow. I first justified this guard by claiming set -e does not fire on a failed command substitution in an assignment. That was wrong, and CodeRabbit caught it. The masking applies to declaration assignments, not plain ones:

   $ bash -c 'set -e; x=$(false); echo REACHED'
   exit=1          # REACHED never printed
   
   $ bash -c 'set -e; f(){ local x=$(false); echo REACHED; }; f'
   REACHED
   exit=0
   

   So the step aborts on a failed git diff with or without the guard, and the "empty list reads as nothing changed" scenario I described could not occur. The guard stays because it names which refs the diff was between instead of dying with a bare non-zero exit and git's stderr — a real improvement, just not the one I claimed. Corrected in e0c835a; every line of that commit's diff is a comment.

   script/ci-changes-detector carries the same inaccurate claim, which is where I copied it from. Deliberately not corrected here (it would pull an unrelated file into a two-file PR); recorded on CI: extract ci-required diff-base selection from workflow YAML into script/ #4824 so the extraction fixes both, since the bad reasoning has already propagated once.

9. Two plausible shell claims in this PR were wrong, and CI could not have caught either. Worth stating plainly for whoever maintains this next. The first: that widening the diff base fixes under-selection — false, because the base is only half the diff and HEAD's tree was the broken half (item 6). The second: the set -e claim above. Both survived a fully green hosted matrix, because neither is observable from a passing test — the code did the right thing for a misstated reason, or the failure mode was unreachable. Both were caught by review. The committed harness in item 10 raises the floor for behavior, but reasoning stated in comments still needs a reader.

10. Test harness committed; extracting the step into script/ deferred. Review correctly noted that every other piece of CI-selection logic here pairs with a companion test, and this branching had none. script/ci-required-diff-base-test.bash now covers it, and is wired into this workflow's own Run CI gate tests step. The structural half — moving the logic out of YAML into script/ — is deliberately not done here: refactoring a required gate is a larger change than the bug fix that surfaced it. Filed as CI: extract ci-required diff-base selection from workflow YAML into script/ #4824, proposed alongside CI: nine hosted workflows feed the changed-files detector a stale pull_request.base.sha #4818 since that issue would otherwise paste the same shape into nine more workflows.

Confidence note

• Validated:

  • bash script/ci-required-diff-base-test.bash: 6 diff-base tests passed. Also confirmed to fail when each fix is reverted (output above), so it is a real guard.

  • RUN_GENERATORS=true SHOULD_RUN_HOSTED_CI=false bash script/ci-required-hosted-gate exits 1 with the generator message. This is the evidence behind rejecting the maximal-classification alternative in decision log item 6.

  • bash -n script/ci-required-diff-base-test.bash: syntax OK. shellcheck -S warning: clean.

  • bundle exec rspec spec/react_on_rails/ruby_version_support_spec.rb: 7 examples, 0 failures, including the merge-queue base-SHA guard this PR previously broke.

  • SHELLCHECK_OPTS="-S warning" actionlint repo-wide, exit 0 (also run against the committed file alone, exit 0). This is the same invocation .github/workflows/actionlint.yml uses.

  • ruby -e "... YAML.safe_load_file ..." over all workflows: all workflow YAML parsed OK. This is the gate's own "Validate workflow YAML" step.

  • node .github/workflows/hosted-ci-safety.test.cjs: hosted CI workflow safety tests passed. This test asserts against ci-required.yml directly.

  • ruby bin/lint-mirrored-blocks: 2 MIRROR OF: markers across 1 pair, 10 MIRROR VALUES OF: markers across 5 pairs, exit 0.

  • bash script/ci-required-hosted-gate-test.bash: 7 tests passed.

  • ruby script/ci_required_merge_group_gate_test.rb: 11 tests passed.

  • ruby script/pr_merge_ledger_test.rb: 254 runs, 0 failures.

  • ruby .agents/bin/agent_workflow_drift_manifest_test_test.rb: 11 runs, 0 failures.

  • ruby .agents/skills/pr-batch/bin/pr-ci-readiness-test.rb: 66 runs, 0 failures.

  • node --test .github/workflows/ci-commands.test.cjs: 49 pass, 0 fail.

  • Reproduction and all four selection branches, run against the step body extracted from the committed YAML (output above).

  • Observed on a real GitHub-hosted runner. pull_request workflows run from the PR branch, so this PR's own ci-required run already executed the new step. From run 30455776751:

    Diff base: 822b4c30bc3764a08454a7e891834b2699921a03 (source: PR merge commit first parent)
    Base: 822b4c30bc3764a08454a7e891834b2699921a03 | Current: HEAD | Merge base: 822b4c30bc3764a08454a7e891834b2699921a03
    Changed files the classification was computed from (first 50):
    .github/workflows/ci-required.yml
    

    The two-parent detection, the first-parent selection, and the new changed-file listing all work on ubuntu-latest with the runner's real shallow checkout.

  • Hosted actionlint and zizmor checks both pass on this PR.

• Evidence: failing run https://github.com/shakacode/react_on_rails/actions/runs/29665416514/job/88134918712, passing post-rebase run https://github.com/shakacode/react_on_rails/actions/runs/29665576745/job/88135331590, this PR's hosted run https://github.com/shakacode/react_on_rails/actions/runs/30455776751, plus the inline output above.
• UNKNOWN:
  • No Ruby changed, so bundle exec rubocop was not run. Nothing in the diff is Ruby.
  • The stale-base path itself has not been exercised on hosted CI. This PR was freshly pushed, so its base.sha was current and old and new logic agree on it. The hosted run above proves the new code path executes correctly, not that it repairs a stale branch. Follow-up: Exercise GitHub Actions changes from PR #4819 #4820 specifies the throwaway stale docs-only PR needed to close that gap post-merge.
  • Whether github.event.pull_request.head.sha is ever empty on a real pull_request event is assumed, not observed; the code treats an empty value as "skip the staleness check" rather than failing.
  • The || { ::error::; exit 1; } guard on the changed-file assignment (item 8) has no executable test. Lower stakes than when I first wrote this, since set -e aborts there regardless and the guard only improves the message. Exercising it needs a git that fails only on diff --name-only, and injecting one via PATH did not reliably reach the child shell in my environment, so any case I wrote would have passed or failed for reasons unrelated to the guard. I removed it rather than ship a test that is green for the wrong reason; the gap is recorded in the harness and tracked with CI: extract ci-required diff-base selection from workflow YAML into script/ #4824, where the logic moves into script/ and the guard becomes directly callable. The equivalent guard in script/ci-changes-detector is likewise enforced by comment rather than by test.
  • How often a stale merge ref occurs in practice is unmeasured. If it turns out to be common rather than a rare race, the fail-closed choice in item 6 should be revisited in favor of fetching and classifying the reported head.
  • I did not observe a real conflicted PR producing a single-parent HEAD on this repo. That state is asserted from GitHub's documented merge-ref behavior.
• Residual risk: low and one-directional. If the two-parent guard ever fails to match, the step falls back to exactly today's behavior plus a warning, so the worst case is the status quo rather than a new failure mode.

Detector classification for this PR

script/ci-changes-detector origin/main on this branch reports CI infrastructure (full test suite, no benchmarks) and sets run_generators=true, so ci-required will fail until hosted CI is requested. That is expected for any .github/workflows/** change and is not a symptom of this fix.

Guard-spec failure: diagnosis (coordinator)

The full hosted matrix caught a real failure on the previous head — recorded here because the
authoring agent's session terminated on an API error before it could write this up. The diagnosis
below is the coordinator's own, verified against the committed YAML.

Failure: react_on_rails/spec/react_on_rails/ruby_version_support_spec.rb:116,
"uses the merge queue base SHA for merge_group change detection", on both
rspec-package-tests (3.3, minimum, unit) and (4.0, latest, unit). Job:
https://github.com/shakacode/react_on_rails/actions/runs/30455997425/job/90589676685

Cause: formatting, not behavior. That spec asserts the raw file text of ten workflows contains
github.event.pull_request.base.sha || github.event.merge_group.base_sha || github.event.before || 'origin/main'
as a contiguous string. The first revision of this PR moved that expression into an EVENT_BASE_REF
env var and wrapped it across lines in a YAML folded scalar, so the substring match failed even
though the expression was semantically identical.

The folded scalar was independently wrong: in YAML, continuation lines indented more than the
first content line are preserved literally rather than folded, so the wrapped form would have carried
embedded newlines into the value rather than collapsing to one line.

Fix (021e584a8): the workflow, not the spec. The expression is restored to a single line,
character-for-character what the guard expects, with an inline comment recording why it must stay
that way. The guard was deliberately not weakened or rewritten to match new behavior.

Merge-queue behavior is preserved, and that was verified before accepting the fix:

• base_ref="${EVENT_BASE_REF}" remains the default, so a merge_group event still resolves its
  base through github.event.merge_group.base_sha exactly as before.
• The new first-parent logic is gated behind elif [ "${GITHUB_EVENT_NAME}" = "pull_request" ], so
  it cannot affect the merge queue path at all.
• The two-parent guard ([ "${#parents[@]}" -eq 3 ] against git rev-list --parents -n 1 HEAD)
  means a single-parent HEAD never takes HEAD^1.

This is why hosted CI was requested as +ci-force-full rather than +ci-run-hosted. This PR changes
the base ref that feeds suite selection, so optimized selection would have been grading its own
homework — and the guard spec that caught this is one an optimized run would most likely have
skipped.

UNKNOWN carried forward: the stale-base path itself is still not exercised on hosted CI. This
branch's base.sha is current, so old and new logic agree on it; the hosted run proves the new step
executes correctly, not that it repairs a stale branch. #4820 exists to close that gap post-merge.

Batch: ror-a-17-1-wave-a-20260729, lane lane-4756-ci-mergebase.

Summary by CodeRabbit

• Bug Fixes

  • Improved CI changed-file detection by correctly resolving diff base for pull requests, workflow dispatches, and merge groups.
  • Added safety checks for stale or unexpected merge commit shapes and stricter failure behavior when mismatches are detected.
  • Enhanced CI output by printing the exact changed-file set used for classification (up to 50 files), with clearer handling when no changes are found.

• Tests

  • Added a CI diff-base validation harness covering multiple event scenarios, edge cases, and output/limit behavior.

Merge qualifications (coordinator)

Release gate: beta (target main): confidence note + green required checks. Both satisfied at
head e0c835ac3edaad83af78041b3b82be8470a739ad — full rollup SUCCESS, zero failing, zero pending.
Release mode: development — the only open tracker (#4806) targets 17.0.1, already shipped.

Merge ledger: --strict --changelog-classification not_user_visible -> complete_allowed: true,
zero violations, zero unknown fields, CI READY, zero unresolved current-head review threads.
No changelog entry: internal CI plumbing, not user-visible, per the repo seam.

Hosted CI: +ci-force-full, deliberately. This PR changes the base ref that feeds suite
selection, so optimized selection would have been grading its own homework. That call was vindicated:
the full matrix caught a guard-spec failure (ruby_version_support_spec.rb:116) that optimized
selection would most likely have skipped.

Review cohort: CodeRabbit APPROVED at this head (superseding an earlier CHANGES_REQUESTED,
resolved by a comment-accuracy fix). Nine threads, zero unresolved. Two advisory threads were
resolved in-thread without a push, per review-loop convergence: the twelve-workflow blast radius
(tracked in #4818) and the eventual-consistency tradeoff on the fail-closed path (accepted and
documented).

Coordinator verification, independent of the authoring lane:

• Confirmed the root cause was not what ci-required: shallow merge-base fallback misclassifies stale docs-only PRs as generator changes #4756 described. Fetch depth was never involved:
  script/lib/git-diff-base already deepens 50 -> 100 -> ... -> --unshallow and hard-fails, so the
  "deepen or fail closed" half was already implemented upstream. The real cause is a stale
  github.event.pull_request.base.sha.
• Confirmed bundle-size.yml:82-99 derives its baseline from the merge commit's first parent, so
  this follows existing repo precedent rather than inventing a pattern.
• Confirmed merge-queue behavior is preserved: EVENT_BASE_REF remains the default, so merge_group
  events still resolve through github.event.merge_group.base_sha, and the first-parent logic is
  gated behind GITHUB_EVENT_NAME = pull_request.
• Confirmed the final commit is comment-only by filtering the diff for non-comment lines (empty
  result), so the fully green matrix at 124b42a7b carries to this head.
• Independently reproduced the set -e semantics behind the reworded comment: a plain assignment
  x=$(false) aborts under set -e; only declaration assignments (local/export/declare) mask
  it. The guard is retained for its actionable ::error:: diagnostic, not for the reason originally
  stated.
• Independently reproduced the hosted-gate behavior that justified fail-closed over conservative
  classification: GITHUB_EVENT_NAME=pull_request RUN_GENERATORS=true SHOULD_RUN_HOSTED_CI=false
  exits 1 with the "Comment +ci-run-hosted" message. Maximal classification would therefore have
  red-gated anyway, with the exact wrong remedy ci-required: shallow merge-base fallback misclassifies stale docs-only PRs as generator changes #4756 exists to eliminate.

Two coordinator corrections are recorded in the decision log, because both were wrong in ways a
future reader would repeat. First, an instruction to warn-and-continue on a stale merge ref; second,
an instruction to fall back to the event base, which addressed the wrong half of the diff — widening
changes what you diff from, while a stale HEAD is what you diff to, and its tree simply cannot
contain the newer head's changes. The lane's evidence overrode the coordinator on the final
mechanism, correctly.

UNKNOWN carried forward:

• The stale-base path is still not exercised on hosted CI. This branch's base.sha is current, so
  old and new logic agree; the hosted run proves the new step executes, not that it repairs a stale
  branch. Follow-up: Exercise GitHub Actions changes from PR #4819 #4820 closes that gap post-merge and is required by .github/read-me.md.
• The real-world frequency of a stale refs/pull/<n>/merge at checkout time is unmeasured. If it
  proves common rather than rare, fetching the reported head becomes a better trade than failing
  closed.
• The changed_files guard has no executable test: injecting a git that fails only on
  diff --name-only did not reliably reach the child shell. The lane removed a case that was
  passing for the wrong reason rather than ship it, and tracked it in CI: extract ci-required diff-base selection from workflow YAML into script/ #4824.
• The installed pack's autonomous-merge-eligibility gate is not adopted by this repository, so it
  cannot verify here by construction.

Batch: ror-a-17-1-wave-a-20260729, lane lane-4756-ci-mergebase. merge_authority: auto_merge_when_gates_pass.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[coderabbitai]

Walkthrough

The required CI workflow now resolves event-specific diff bases, validates pull request merge-parent structure, logs the files used for classification, and runs a synthetic-repository test harness covering the updated behavior.

Changes

CI diff-base resolution

Layer / File(s)	Summary
Resolve and log the detector diff base .github/workflows/ci-required.yml	The workflow selects dispatch, event, or merge-parent bases, rejects stale merge refs, warns on unexpected parent structures, runs the changed-files detector, and prints the classified files.
Validate diff-base behavior script/ci-required-diff-base-test.bash	The harness extracts the workflow step, exercises synthetic merge, stale-ref, event, override, and large-file scenarios, and verifies outputs and exit statuses.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related issues

• #4824 — Concerns extracting and testing the same inline diff-base selection logic.
• #4818 — Addresses stale pull_request.base.sha handling in related workflows.

Suggested reviewers: copilot

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The PR addresses [#4756] by improving merge-ref base selection, warning on single-parent checkouts, failing stale refs, and reporting classified files.
Out of Scope Changes check	✅ Passed	The workflow update and new shell test harness are directly tied to the ci-required base-ref fix and do not appear unrelated.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: fixing ci-required base ref handling for PR merge-ref checkouts.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/4756-ci-required-mergebase

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

Fixes required-CI change classification for pull-request merge checkouts.

• Uses the checked-out two-parent merge commit’s first parent as the diff base.
• Preserves explicit dispatch inputs and existing event-based fallbacks.
• Warns when the checkout is not the expected merge structure or represents a stale PR head.
• Prints the first 50 files used for classification diagnostics.

Confidence Score: 3/5

The stale merge-ref path should be fixed before merging because it can classify an older PR revision and skip required hosted CI for current changes.

A mismatch between the merge commit’s second parent and the event’s current head is detected but only warned about, after which suite routing is still computed from the stale checkout.

Files Needing Attention: .github/workflows/ci-required.yml

Important Files Changed

Filename	Overview
.github/workflows/ci-required.yml	Derives the pull-request diff base from the merge checkout’s first parent, but continues classification against an older checkout when the merge ref is stale.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Select default base from event payload] --> B{Explicit workflow_dispatch base?}
  B -->|Yes| C[Use workflow_dispatch input]
  B -->|No| D{pull_request event?}
  D -->|No| E[Use event payload base]
  D -->|Yes| F{HEAD has exactly two parents?}
  F -->|Yes| G[Use HEAD first parent]
  G --> H{Second parent matches PR head?}
  H -->|No| I[Emit stale merge-ref warning]
  H -->|Yes| J[Run change detector]
  I --> J
  F -->|No| K[Warn and retain event payload base]
  K --> J
  C --> J
  E --> J
  J --> L[Print first 50 classified files]

Loading

_{Reviews (1): Last reviewed commit: "Fix ci-required base ref for PR merge-re..." | Re-trigger Greptile}

[justin808]

+ci-force-full

Batch coordinator (ror-a-17-1-wave-a-20260729, lane lane-4756-ci-mergebase). Requesting force-full rather than optimized hosted CI deliberately.

This PR changes the base ref that feeds script/ci-changes-detector, so optimized suite selection is the thing under test. Under +ci-run-hosted the detector chooses the suites, which means a defect in path selection could conceal itself by selecting fewer suites — the gate would be grading its own homework. Forcing the full matrix removes that circularity.

Per AGENTS.md → PR CI Labels, force-full is reserved for cases where "path selection itself is part of the risk," which is exactly this. Cost is one full run on a single-file workflow change.

The detector classifies this PR run_generators=true, so required-pr-gate is expected to fail until hosted CI completes.

[github-actions]

Force-Full Hosted CI Requested

Triggered 9 workflow(s) for f0b725f8d358.
Skipped 0 workflow(s) with equivalent exact-head coverage.
Mode: force-full hosted CI (bypasses optimized change selection).
Added ready-for-hosted-ci and force-full-hosted-ci, so future commits will bypass optimized hosted CI selection until +ci-stop-full is used.

View progress in the Actions tab.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f0b725f8d3

ℹ️ 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".

[claude]

Review: Fix ci-required base ref for PR merge-ref checkouts

Overview

Single-file change to .github/workflows/ci-required.yml's "Run changed-files detector" step. Instead of always diffing from github.event.pull_request.base.sha (which only refreshes on PR open/synchronize and can drift from the checked-out refs/pull/<n>/merge commit's actual base parent on long-lived branches), the step now:

• Reads HEAD's parents via git rev-list --parents -n 1 HEAD.
• On a pull_request event with a genuine two-parent merge commit, uses the first parent as the diff base (mirrors the existing precedent in bundle-size.yml).
• Warns (::warning::) but keeps the stale event-payload base when HEAD isn't a two-parent merge commit (e.g., a conflicted PR with no merge ref) or when the merge ref's second parent doesn't match the current PR head.
• Prints the resolved base, its source, and the first 50 changed file paths for visibility.

The PR body is unusually thorough (root-cause repro on a full clone, a 4-branch test matrix, extensive test-suite validation, and an explicit decision log), and the fix is narrowly scoped — no changes to script/ci-changes-detector itself, fetch-depth left at 50 with sound justification (the first parent of HEAD can never fall outside the shallow window).

Code quality

• Well-commented: each non-obvious choice (why first-parent, why warn instead of hard-fail, why fetch-depth is untouched) has an inline rationale.
• Correctly avoids embedding untrusted ${{ }} expressions directly in the run: body — everything goes through env: first, which is the standard mitigation for the GitHub Actions script-injection class of bug.
• read -ra parents <<< "$parent_line" combined with the -eq 3 / -eq checks degrades safely: any unexpected parent count (0, 1, or >2) falls through to the existing warn-and-fallback path rather than erroring.
• Defensive design around set -e: the git rev-list ... || true and the if diff_base=$(...); then pattern correctly avoid tripping -e on expected failure paths.

Issue found (see inline comment)

• git diff --name-only "$diff_base" HEAD | head -50 on line 185 is a plain statement inside the if body, so it's still subject to the step's set -eo pipefail. If the diff has enough output to fill the pipe buffer before git diff finishes writing (a large changed-file count), head -50 can close early, git diff gets SIGPIPE, and under pipefail that failure propagates and fails the whole step — even though this line is purely informational. This is most likely to bite in exactly the "stale base / huge diff" scenario the PR is designed to surface. Suggest | head -50 || true or writing to a temp var/file first.

Other observations (not blocking)

• The acknowledged residual gap — the warn path still uses the stale base.sha for genuinely conflicted PRs — is reasonable given ci-required is a required gate and a hard fail there would be strictly worse than today's status quo. Well-reasoned tradeoff, clearly called out in both the code comment and PR description.
• base_source="event payload" is a shared label for three different origins (base.sha, merge_group.base_sha, github.event.before); minor readability nit only, no functional impact.
• The added git merge-base "$base_ref" HEAD + git diff --name-only at the end re-derives information the detector already computed internally, purely for display. Cheap and already called out as no extra network cost — fine as is.

Security

No new injection surface: all event-derived values flow through env: vars before use in the shell script, and the workflow's permissions block stays read-only. No concerns.

Overall: solid, well-justified fix with strong test coverage described in the PR body. One real (if edge-case) robustness bug on the new diagnostic line worth fixing before merge.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[claude]

Review summary

What this PR does: Fixes ci-required.yml's changed-file classification for pull_request events. Previously the workflow diffed against github.event.pull_request.base.sha, which only refreshes on open/synchronize and can drift from the actual base once HEAD (the checked-out refs/pull/<n>/merge commit) is recomputed against a newer base tip — folding intervening base-branch commits into the "changed files" set and misrouting docs-only PRs into the full hosted suite. The fix reads HEAD's parents via git rev-list --parents -n 1 HEAD; when it's a genuine two-parent merge commit, it uses the first parent (the actual merge base) instead of the stale SHA, and emits a ::warning:: if the second parent doesn't match the current PR head (stale merge ref) or if HEAD isn't a two-parent commit at all (falls back to old behavior). fetch-depth is intentionally left at 50 since the first parent is always already present locally.

Correctness: The core logic checks out. I traced the branches against script/ci-changes-detector / script/lib/git-diff-base (untouched by this PR) and confirmed:

• The merge_group/workflow_dispatch paths are unaffected (the new branch only fires when GITHUB_EVENT_NAME == pull_request and no dispatch SHA input was given).
• parents[1]/parents[2] indexing and the #parents[@] -eq 3 guard correctly gate the fast path only for genuine 2-parent merge commits; anything else (0, 1, or >2 tokens from a failed/odd rev-list) falls through to the safe warn-and-fallback path.
• The precedent comment pointing at bundle-size.yml:82-99 for the "first parent = actual base" technique checks out — that workflow uses the identical git rev-list --parents + array-index pattern, just hard-failing instead of warning (justified in the PR body: this is a required gate, that one is advisory).
• The comment claiming the one-line EVENT_BASE_REF expression is asserted verbatim by ruby_version_support_spec.rb is accurate — that spec does a literal substring include(...) check against the raw workflow YAML text, so reformatting it (e.g., a YAML folded scalar) would break that spec.
• Both parent-side and stale-merge-ref warnings intentionally still use the first parent as the base (per the PR's decision log) rather than silently falling back further — reasonable, since the first parent remains the exact tree that was checked out either way.

Main concern — test coverage: left as an inline comment. This PR adds meaningful branching logic (array parsing off git rev-list --parents, three distinct code paths) directly inside the workflow YAML run: block, with zero automated test coverage, breaking from this repo's own convention of extracting CI-selection logic into script/* files with dedicated *-test.bash/*_test.rb companions. The PR's validation section is thorough (existing test suites, actionlint, live hosted-CI run) but none of it actually asserts this new branching logic in isolation — the two-parent happy path is implicitly exercised by this PR's own CI run, but the single-parent and stale-second-parent branches are only manually reproduced outside the committed file, not unit tested.

Security: No concerns — all interpolated values are either GitHub-provided SHAs or output of local git rev-list, none of it attacker-controllable in a way that reaches an unsafe sink (no eval/exec of the values, just string comparison and git merge-base/git diff calls).

Performance: None — no extra network calls (git merge-base/git diff --name-only both operate on already-fetched local history), and fetch-depth is correctly left unchanged.

Scope: Appropriately narrow — only ci-required.yml touched, with the other nine workflows sharing the same stale-base pattern explicitly deferred to #4818 rather than scope-creeping this PR.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b2d695aca0

ℹ️ 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".

[claude]

Review

Went through the diff-base logic in .github/workflows/ci-required.yml and the new script/ci-required-diff-base-test.bash harness in detail, including tracing script/ci-changes-detector → script/lib/git-diff-base to confirm the follow-on git merge-base "$base_ref" HEAD diagnostic call reuses history already fetched/deepened by the detector (so it can't itself fail from missing history), and confirming EVENT_BASE_REF's single-line form still matches the exact string asserted by ruby_version_support_spec.rb:116-139.

Correctness: The core fix is sound. HEAD's first parent (git rev-list --parents -n 1 HEAD, guarded on exactly 3 tokens) is definitionally the base tip GitHub actually merged against for refs/pull/<n>/merge, so it can never omit real PR changes the way a stale base.sha can only ever widen the diff, never narrow it — the security-relevant property (a required gate must not be narrowable to skip generator tests) holds in all four branches (fresh merge ref, stale merge ref, single-parent/conflicted HEAD, merge_group/workflow_dispatch). The stale-merge-ref and single-parent fallbacks both degrade to widening, not narrowing, which is the correct fail-safe direction for a required gate.

Security: Moving the github.event.* interpolations out of the inline ${{ }} in the run: body and into env: (EVENT_BASE_REF, PULL_REQUEST_HEAD_SHA) is a good defense-in-depth improvement even though these particular values are always SHAs today.

Testing: script/ci-required-diff-base-test.bash extracts the actual step body from the committed YAML via a YAML parser rather than hand-copying it, so the tests can't drift from what ships, and it fails loudly if the step is renamed. It also covers a real regression (SIGPIPE/exit 141 under pipefail when head -50 closes the pipe early on a large diff) with a synthetic 3000-file case.

One nit posted inline: the new diagnostic changed_files=$(git diff --name-only "$diff_base" HEAD) line doesn't have the same failure guard that script/ci-changes-detector explicitly requires (and documents) for the identical set -e-doesn't-fire-on-assignment pitfall. Low severity since this block only prints a human-readable file list after the real classification/outputs have already been produced, but worth the same guard for consistency.

No other correctness, security, or performance issues found. The documented residual gap (stale merge-ref fallback path not yet exercised on hosted CI) is already called out in the PR description and tracked via #4820, so I'm not re-flagging it here.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[claude]

Review: Fix ci-required base ref for PR merge-ref checkouts

Overview

This PR fixes a real bug: ci-required.yml's changed-files detector diffed from github.event.pull_request.base.sha, which only refreshes on PR open/synchronize and can drift from the merge commit actions/checkout actually resolves for pull_request events. The fix takes the base from the merge commit's first parent (mirroring the precedent in bundle-size.yml), guarded on HEAD having exactly two parents, with a workflow_dispatch/merge_group override chain preserved. A new test harness (script/ci-required-diff-base-test.bash) extracts the step body straight from the committed YAML and exercises it against a synthetic repo under the same shell Actions uses — good practice, and it demonstrably catches the SIGPIPE/head -50 regression the PR fixes along the way.

The investigation and write-up are unusually rigorous (reproduced on a real runner, verified the fix fails when reverted, checked shellcheck/actionlint/YAML parsing). The one substantive concern below is that the shipped code has drifted from what the write-up says it does.

Main finding: description/code mismatch on the stale-merge-ref path (also left as an inline comment)

.github/workflows/ci-required.yml lines ~162–185: when the merge commit's second parent doesn't match github.event.pull_request.head.sha (a stale refs/pull/<n>/merge), the code emits ::error:: and exit 1, hard-failing the required gate.

The PR's own "Codex Decision Log" item 6 describes different behavior: falling back to EVENT_BASE_REF with a warning (base_source="event payload (merge ref stale)"), reasoned through at length as "the only defensible move." That fallback doesn't exist in the diff — the test is named "stale merge ref fails closed", not "falls back." Please reconcile these:

• If fail-closed is intentional, the PR description/decision log should say so, since it currently documents a design the code doesn't implement.
• Worth reconsidering whether fail-closed is right for a required gate: the condition is explicitly described as a GitHub-side race ("a re-run clears it") that can occur on ordinary synchronize events, not just pathological cases. Hard-failing means a red required check with no relation to the contributor's actual change, and on fork PRs, re-running a failed workflow run may require maintainer access. The EVENT_BASE_REF fallback the description originally proposed doesn't have this problem — it isn't derived from HEAD's tree, so it's available regardless of merge-ref staleness, and it's the same base the single-parent branch already trusts.
• The PR's "Residual risk: low and one-directional... worst case is the status quo" claim only analyzes the single-parent branch; it doesn't cover this exit-1 path, which is a genuinely new failure mode (workflow failure requiring a manual re-run) rather than a fallback to prior behavior.

Other observations

• Security: untrusted event data (pull_request.head.sha, base.sha) is correctly passed through env: rather than interpolated directly into the run: script, avoiding the classic Actions script-injection pattern. Good.
• Correctness: the two-parent guard (#parents[@] -eq 3), the workflow_dispatch/merge_group precedence order, and the sed -n '1,50p' fix for the head -50/SIGPIPE/pipefail failure mode all look correct and are exercised by the new test's 6 cases.
• Test coverage: solid for the paths it covers (current merge ref, single-parent HEAD, workflow_dispatch input, merge_group, large diff/truncation). No coverage for an octopus merge (3+ parents) on HEAD, but that shape doesn't occur for GitHub PR merge refs in practice, so this is minor.
• Performance: fetch-depth intentionally left at 50 since the first parent is guaranteed to be within the shallow window; the extra git merge-base/git diff --name-only calls reuse history the detector already fetched, so no added network cost. Reasoning checks out.
• Scope: appropriately narrow — two files, no unrelated refactoring, with follow-up issues (CI: nine hosted workflows feed the changed-files detector a stale pull_request.base.sha #4818, CI: extract ci-required diff-base selection from workflow YAML into script/ #4824, Follow-up: Exercise GitHub Actions changes from PR #4819 #4820) filed for adjacent work rather than scope creep into this PR.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[claude]

Review

Focused on .github/workflows/ci-required.yml and script/ci-required-diff-base-test.bash.

Code quality / correctness

The bash logic itself is solid:

• The two-parent guard (${#parents[@]} -eq 3) correctly distinguishes a real PR merge commit from a single-parent HEAD or an unexpected (e.g. octopus) merge shape, and safely falls back to the event base with a warning in the latter cases.
• Using EVENT_BASE_REF/PULL_REQUEST_HEAD_SHA as env: context (rather than interpolating ${{ github.event.* }} directly into the run: script body, which the old code did for base_ref) is a good, if incidental, security hardening — it removes a script-injection vector for untrusted github.event.* strings. Not strictly relevant here since these are GitHub-controlled SHAs, but good practice.
• The sed -n '1,50p' instead of head -50 fix, and the || { echo ::error::; exit 1; } guard on the changed_files assignment, are both correct and well-reasoned given bash -eo pipefail semantics.
• script/ci-required-diff-base-test.bash is a legitimate, self-executing test (extracts the real step body from the committed YAML via a YAML parser and runs it under the same shell Actions uses), and the assertions I traced through (first-parent selection, single-parent fallback, workflow_dispatch precedence, merge_group untouched, large-file-list truncation) check out.

Main finding: PR description doesn't match the shipped behavior for the stale-merge-ref case (left as an inline comment on the relevant block)

The code hard-fails (::error:: + exit 1) when the merge ref's second parent doesn't match github.event.pull_request.head.sha (a stale merge ref). But the PR description — in "The fix is not total, by design," in Decision log item 6, and in the "Branch matrix" repro output — describes and shows a warn-and-fallback-to-EVENT_BASE_REF behavior for this exact case, including a base_source="event payload (merge ref stale)" string that doesn't appear anywhere in the diff. The actual test case is even named "stale merge ref fails closed" (asserting exit 1), not "stale merge ref falls back to the event base" as the body's repro block claims.

This reads like leftover text from an earlier iteration (item 6 mentions being "reversed in review" once already) that wasn't updated after the final change to fail-closed. It's worth reconciling before merge: fail-closed is a materially different — and more disruptive — behavior for a required gate (it can hard-block a PR on a transient GitHub race rather than just widening the diff and warning), and reviewers should be evaluating the behavior that's actually shipping, not the one described.

Minor

• The "Changed files... (${changed_total} total, showing up to 50)" message is printed even when the total is well under 50, which reads slightly oddly (e.g. "10 total, showing up to 50") — cosmetic only, not worth blocking on.
• The post-detector git merge-base "$base_ref" HEAD / git diff block silently no-ops if merge-base fails (the if diff_base=...; then ... fi has no else), so a merge-base failure there produces no changed-file listing and no diagnostic. Low risk since script/ci-changes-detector (called just before, with the same base_ref) would already have failed loudly if the base were genuinely unreachable.

Nothing else here raises correctness, security, or performance concerns — the shell-injection avoidance, pipefail/SIGPIPE fix, and fail-closed guard for under-selection are all good calls. The main ask is to fix the description/behavior mismatch above.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

.github/workflows/ci-required.yml (2)

206-234: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Silent skip when git merge-base fails.

If git merge-base "$base_ref" HEAD fails, the whole diagnostic block is skipped with no ::warning::/::error::, so the log just ends after the "Diff base: ..." line with no explanation for the missing file listing. Given how carefully every other failure path in this step is surfaced, consider emitting a warning here too for consistency.

♻️ Suggested diagnostic on merge-base failure

-          if diff_base="$(git merge-base "$base_ref" HEAD 2>/dev/null)"; then
+          if diff_base="$(git merge-base "$base_ref" HEAD 2>/dev/null)"; then
             ...
+          else
+            echo "::warning::Could not compute merge-base of ${base_ref} and HEAD; skipping changed-file listing."
           fi

🤖 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-required.yml around lines 206 - 234, Add an explicit
failure branch for the git merge-base command surrounding changed_files in the
diagnostic block, emitting a clear ::warning:: or ::error:: that the
changed-file listing was skipped because merge-base failed. Preserve the
existing successful-path diff handling and git diff failure guard.

---

129-235: 🧹 Nitpick | 🔵 Trivial

Reminder: this is a semantic workflow change.

This reworks the diff-base logic that drives suite selection for every PR. As per coding guidelines, for semantic workflow changes you should "inspect secrets, permissions, triggers, and third-party actions; run actionlint, yamllint .github/, and script/ci-changes-detector origin/main, and post a Workflow Change Audit: summary before merge."

🤖 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-required.yml around lines 129 - 235, Before merging the
semantic diff-base changes in the PR workflow, inspect its secrets, permissions,
triggers, and third-party actions; run actionlint, yamllint .github/, and
script/ci-changes-detector origin/main; then post a “Workflow Change Audit:”
summary containing the findings and validation results.

Source: Coding guidelines

🤖 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-required.yml:
- Around line 216-225: Update the comment immediately above the changed_files
assignment in the workflow to accurately describe Bash set -e behavior for this
plain assignment. Remove the claim that a failed git diff would be silently
suppressed, and retain the rationale that the explicit guard provides a clear
::error:: message before exiting; preserve the guard and its behavior unchanged.

---

Nitpick comments:
In @.github/workflows/ci-required.yml:
- Around line 206-234: Add an explicit failure branch for the git merge-base
command surrounding changed_files in the diagnostic block, emitting a clear
::warning:: or ::error:: that the changed-file listing was skipped because
merge-base failed. Preserve the existing successful-path diff handling and git
diff failure guard.
- Around line 129-235: Before merging the semantic diff-base changes in the PR
workflow, inspect its secrets, permissions, triggers, and third-party actions;
run actionlint, yamllint .github/, and script/ci-changes-detector origin/main;
then post a “Workflow Change Audit:” summary containing the findings and
validation results.

🪄 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 Plus

Run ID: c2511d33-5ef8-4bcd-b59a-b8f4bb952444

📥 Commits

Reviewing files that changed from the base of the PR and between f0b725f and 124b42a.

📒 Files selected for processing (2)

• .github/workflows/ci-required.yml
• script/ci-required-diff-base-test.bash

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[claude]

Review summary

Solid, well-reasoned fix for the actual root cause identified in #4756 (stale pull_request.base.sha vs. the merge commit's own base parent), with good evidence in the PR description and a dedicated test harness (script/ci-required-diff-base-test.bash) that extracts and exercises the real YAML step body against a synthetic repo. A few things worth calling out (also left as inline comments):

Main finding — the fix is scoped to one workflow, but the bug is duplicated in ~12 others. gem-tests.yml, playwright.yml, integration-tests.yml, lint-js-and-ruby.yml, precompile-check.yml, package-js-tests.yml, pro-integration-tests.yml, pro-test-package-and-gem.yml, examples.yml, check-docs-sidebar.yml, check-llms-full.yml, and actionlint.yml all fire directly on pull_request and independently recompute a base ref using the exact same stale-base.sha pattern this PR fixes only in ci-required.yml. So the specific suites that actually run tests/lint can still misclassify a docs-only (or otherwise narrow) PR the same way #4756 described, regardless of what the required gate now decides. Since the diff-base logic is meant to eventually live in script/ (per the note in the new test file referencing #4824), that might be the natural point to fix every call site at once rather than leaving the other workflows on the old logic in the meantime.

Security note (positive): the old code interpolated ${{ github.event.pull_request.base.sha || ... }} directly into the run: script body — a pattern that's normally flagged as a GitHub Actions script-injection risk. This PR moves it to an env: var (EVENT_BASE_REF) instead, which is the right fix regardless of the low practical risk here (these values are commit SHAs). Good catch/cleanup.

Design tradeoff worth surfacing: the new fail-closed path (exit 1 when the merge commit's second parent doesn't match github.event.pull_request.head.sha) makes the required gate hard-fail on a known GitHub eventual-consistency race (merge ref not yet recomputed), rather than only on genuinely stale branches. This mirrors existing precedent in bundle-size.yml, and the reasoning for failing closed over silently widening the diff is sound — just flagging that maintainers may occasionally see this required check go red with "re-run and it clears" as the only fix, which isn't obvious from the error text alone without having read this PR's reasoning.

No correctness issues found in the parent-parsing logic itself (git rev-list --parents -n 1 HEAD / array indexing), the merge_group and workflow_dispatch precedence handling, or the SIGPIPE-avoidance rationale for the changed-files preview block — the test cases in ci-required-diff-base-test.bash cover the important branches (current merge ref, stale merge ref, single-parent HEAD, workflow_dispatch override, merge_group, and the large-diff truncation path) and match the implementation.