Skip to content

Replace helper binstub sync note - #1183

Merged
justin808 merged 2 commits into
mainfrom
codex/b2-binstub-1149
Jul 1, 2026
Merged

Replace helper binstub sync note#1183
justin808 merged 2 commits into
mainfrom
codex/b2-binstub-1149

Conversation

@justin808

@justin808 justin808 commented Jul 1, 2026

Copy link
Copy Markdown
Member

Summary

  • Replace the maintainer-only sync reminder in distributed/generated helper binstubs with a user-facing regeneration note.
  • Keep the generated template and checked-in helper binstubs in parity.
  • Add test coverage so generated binstubs do not expose the internal package sync path.

Closes #1149

Validation

  • PATH=/Users/justin/.local/share/mise/installs/node/22.20.0/bin:/Users/justin/.local/share/mise/installs/yarn/1.22.22/bin:$PATH yarn test test/configExporter/createBinStub.test.js --runInBand
  • PATH=/Users/justin/.local/share/mise/installs/node/22.20.0/bin:/Users/justin/.local/share/mise/installs/yarn/1.22.22/bin:$PATH bundle exec rspec spec/shakapacker/binstub_sync_spec.rb
  • PATH=/Users/justin/.local/share/mise/installs/node/22.20.0/bin:/Users/justin/.local/share/mise/installs/yarn/1.22.22/bin:$PATH yarn lint
  • codex review --base origin/main clean

Notes

  • .agents/bin/validate was attempted; RuboCop completed with no offenses, then the wrapper entered a local mise node@22.20.0 install/npm -v hang. Targeted Ruby and JS checks above were run with the direct Node/Yarn path.

Summary by CodeRabbit

  • Chores

    • Updated binstub header text to clearly state the files are managed by Shakapacker and should be regenerated through the install/init command.
    • Applied the same wording across generated and checked-in binstub files for consistency.
  • Tests

    • Expanded coverage to verify the updated notice appears in generated binstubs and the old helper-sync message is no longer present.

@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@justin808, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 41 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b7ed68cc-b39c-4db4-858b-71715bb9c500

📥 Commits

Reviewing files that changed from the base of the PR and between d01cc35 and 487a3d5.

📒 Files selected for processing (5)
  • lib/install/bin/diff-bundler-config
  • lib/install/bin/shakapacker-config
  • package/configExporter/cli.ts
  • spec/dummy/bin/shakapacker-config
  • test/configExporter/createBinStub.test.js

Walkthrough

The maintainer-only "keep helper logic in sync" comment in four synchronized binstub files/templates is replaced with a user-facing "managed by Shakapacker" regeneration notice. The associated parity test is updated to assert the new comment's presence and the old comment's absence.

Changes

Binstub Header Comment Update

Layer / File(s) Summary
Replace maintainer sync comment with user-facing notice
lib/install/bin/diff-bundler-config, lib/install/bin/shakapacker-config, spec/dummy/bin/shakapacker-config, package/configExporter/cli.ts
Header comment referencing cross-file sync tracking is replaced with a comment stating the binstub is managed by Shakapacker and should be regenerated via install/init, applied identically across all four synchronized copies including the createBinStub template.
Update parity test assertions
test/configExporter/createBinStub.test.js
Test now asserts the generated binstub includes the "managed by Shakapacker" comment and does not include the old "Keep helper logic in sync across" text referencing package/configExporter/cli.ts.

Estimated code review effort: 1 (Trivial) | ~5 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: replacing the helper binstub sync note.
Linked Issues check ✅ Passed The PR updates all four synchronized binstub copies and strengthens parity tests as requested in #1149.
Out of Scope Changes check ✅ Passed The changes stay within the binstub comment rewrite and its parity test coverage, with no obvious unrelated scope.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/b2-binstub-1149

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

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

@greptile-apps

greptile-apps Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR updates the helper binstub note shown in distributed and generated files. The main changes are:

  • Replaces the internal sync reminder with a user-facing regeneration note.
  • Keeps the checked-in helper binstubs and generated template text aligned.
  • Adds tests that assert generated binstubs omit the internal package sync path.

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.

Important Files Changed

Filename Overview
lib/install/bin/diff-bundler-config Updates the binstub comment without changing executable Ruby behavior.
lib/install/bin/shakapacker-config Updates the binstub comment without changing executable Ruby behavior.
spec/dummy/bin/shakapacker-config Keeps the dummy binstub copy aligned with the installed binstub comment.
package/configExporter/cli.ts Updates the generated binstub template to emit the new regeneration note.
test/configExporter/createBinStub.test.js Adds assertions for the new binstub note and the removal of the internal sync wording.

Reviews (1): Last reviewed commit: "Use user-facing helper binstub comment" | Re-trigger Greptile

Comment thread package/configExporter/cli.ts Outdated
@claude

claude Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review

Small, focused change: replaces the maintainer-only "keep these files in sync" comment inside the four generated/checked-in shakapacker-config / diff-bundler-config binstubs with a user-facing note, since this file ships into end-user apps and previously leaked internal repo paths (e.g. package/configExporter/cli.ts) that mean nothing to consumers.

Verified:

  • All four copies (lib/install/bin/shakapacker-config, lib/install/bin/diff-bundler-config, spec/dummy/bin/shakapacker-config, and the createBinStub template in package/configExporter/cli.ts) are updated consistently and remain byte-for-byte in sync (modulo the documented .cjs script name diff), matching what spec/shakapacker/binstub_sync_spec.rb already enforces.
  • New Jest assertions in test/configExporter/createBinStub.test.js correctly lock in the new text and guard against the old internal-path leak reappearing.
  • No security or performance implications — comment-only change in generated Ruby binstubs.

One accuracy issue (left as an inline comment): the new comment claims "Regenerate it by rerunning the install or init command," but createBinStub() in package/configExporter/cli.ts is only called when the binstub doesn't already exist (!existsSync(binStubPath)), so re-running bin/shakapacker-config --init on an existing stub is a silent no-op — it won't actually regenerate the file. Worth tightening the wording (e.g. mention deleting the file first) so the user-facing instructions are accurate.

Otherwise this looks good — minimal, well-tested, and in line with the project's "convention over configuration" / focused-PR guidelines.

@justin808
justin808 merged commit 1d7a12a into main Jul 1, 2026
16 checks passed
@justin808
justin808 deleted the codex/b2-binstub-1149 branch July 1, 2026 04:54
@claude

claude Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review

Small, well-scoped change — replaces the maintainer-facing "keep these 4 files in sync" comment (which leaked internal repo paths like package/configExporter/cli.ts into binstubs generated inside user apps) with a user-facing regeneration note. Verified:

  • All four copies (lib/install/bin/shakapacker-config, lib/install/bin/diff-bundler-config, spec/dummy/bin/shakapacker-config, and the createBinStub template in package/configExporter/cli.ts) are updated consistently and remain byte-for-byte in sync (per spec/shakapacker/binstub_sync_spec.rb's normalization logic).
  • The actual sync-enforcement mechanism is the existing RSpec/Jest parity tests, not the removed comment, so dropping it doesn't reduce safety.
  • The new test assertions in createBinStub.test.js correctly close the gap called out in the PR description (verifying generated binstubs don't leak the internal sync path).
  • New comment text is accurate: lib/install/bin/* files are copied into bin/ by lib/install/binstubs.rb during the install template (rails shakapacker:install / rake shakapacker:binstubs), and bin/shakapacker-config --init creates the stub via createBinStub when missing.

Minor nits (non-blocking)

No bugs, security issues, or performance concerns found. LGTM.

justin808 added a commit that referenced this pull request Jul 4, 2026
## Summary

Stamps the **`v10.2.0`** release section in `CHANGELOG.md` and adds the
user-visible entries that were still missing for PRs merged since
`v10.1.0`.

Header format matches the repo convention (`## [v10.2.0] - July 3,
2026`) and is parseable by `rakelib/release.rake`'s
`extract_changelog_section` (`## [v<npm-version>]`), so `bundle exec
rake release` / `sync_github_release` will pick up the notes
automatically.

### Changelog changes

- **Version header**: inserted `## [v10.2.0] - July 3, 2026` immediately
after `## [Unreleased]`; all accumulated entries now live under it, and
`## [Unreleased]` is empty.
- **Compare links**: `[unreleased]` now compares `v10.2.0...main`; added
`[v10.2.0]: …/compare/v10.1.0...v10.2.0`.

### New entries added (were missing)

| PR | Section | Note |
| --- | --- | --- |
| [#1187](#1187) | Added |
Babel 8 peer dependency support + preset option compatibility |
| [#1184](#1184) | Added |
Folded into the #695 AI-prompt entry (gates the React on Rails section
on app detection) |
| [#1142](#1142) | Fixed |
Rspack dev-server config no longer loads in static watch mode (fixes
#1137) |

### Already documented (carried into v10.2.0)

`#1180`, `#695`, `#1141`, `#1150`, `#1179`, `#1192`, `#1127`, `#1178`,
`#1161`, `#1147`.

### Reviewed and intentionally excluded (not user-visible)

Docs: `#1145`, `#1148`, `#1152`, `#1155`, `#1183`, `#1188`, `#1189`,
`#1193`.
CI: `#1151`, `#1168`, `#1171`.
Tests / fixtures: `#1128`, `#1154`, `#1167`, `#1186`.
Workflow / agent tooling: `#1153`, `#1176`, `#1182`.

(`#1107` is already documented under `## [v10.1.0]`.)

## Next step

After merge, run the repo's release task (no args) — it reads `v10.2.0`
from the changelog and creates the GitHub release from this section.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
justin808 added a commit that referenced this pull request Jul 6, 2026
## Summary

Adds a repo-owned merge readiness helper and checklist entry so the
current-head merge gate checks the full `gh pr checks` list, unresolved
review threads, mergeability, and replay timing rather than only
required checks.

Closes #1196.

## Validation

- `ruby -c .agents/bin/merge-readiness-check`
- `bundle exec rubocop .agents/bin/merge-readiness-check`
- `.agents/bin/merge-readiness-check 1183 --repo shakacode/shakapacker`
returned `MERGE_READINESS_NOT_READY` and caught 19 checks completing
after the merge timestamp.
- `.agents/bin/merge-readiness-check 1207 --repo shakacode/shakapacker`
returned `MERGE_READINESS_READY` for a clean merged PR.
- `git diff --check origin/main...HEAD`

## Gate / Replay Evidence

- Mechanism target: `checklist+replay`.
- Motivating miss: PR #1183 merged at `2026-07-01T04:54:08Z` before the
full current-head check/review-agent set settled.
- Replay control: `.agents/bin/merge-readiness-check 1183 --repo
shakacode/shakapacker` fails because current-head checks including
`validate`, `test`, `claude-review`, and matrix jobs completed after the
merge timestamp.
- Stale-base control: the new helper lives in `.agents/bin` and the PR
template points future PRs at the current-head command; the historical
replay proves the mechanism catches the motivating stale/current-head
timing miss before this gate text lands.
- Non-goal: this does not make AI reviewers maintainer approvers; it
only requires their configured checks/comments to be completed and
triaged before merge.

## Notes

This is a workflow/helper-script change, not application runtime code.
`CHANGELOG.md` is unchanged because this is process tooling, not a
user-visible package change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Helper binstubs leak maintainer-only sync comment into user projects

1 participant