Walkthrough

Adds a semantic configuration diff workflow: new CLI wrapper bin/diff-bundler-config (and installed copy), a detailed Configuration Diff Guide, docs updates to use semantic diffs, an optionalDependency on pack-config-diff, CI/Rake yalc changes, and doctor/binstub reporting updated to include the new optional binstub.

Changes

Sequence Diagram

sequenceDiagram
    actor User
    participant CLI as "bin/diff-bundler-config"
    participant Local as "pack-config-diff (local)"
    participant NPX as "npx / fetched package"

    User->>CLI: invoke with args
    CLI->>CLI: try require('pack-config-diff')
    alt local module found
        CLI->>Local: load module and validate export
        CLI->>Local: call run(argv)
        Local-->>CLI: return exit code or Promise
    else module not found
        CLI->>NPX: spawn npx pack-config-diff@^0.1.0 with args
        NPX-->>CLI: process exit code
    end
    CLI-->>User: exit with validated integer code (or 2 on error)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐇 I hopped in code with a keen little clap,
I load local first—then npx if it’s gap,
Docs grew a map, doctor nods optional and neat,
Configs now compare cleanly, no brittle repeat,
A carrot for diffs, crunchy and sweet. 🥕

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 @coderabbitai help to get the list of available commands and usage tips.}

Review: Supersede #961 by using pack-config-diff

The approach of extracting the diff engine into a dedicated package is sound, and the binstub wrapper pattern is consistent with existing tooling. A few issues worth addressing before merging:

Security: Unpinned GitHub dependency (blocking)

Two separate places pull from the GitHub repo without a version or commit pin:

1. package.json: the specifier github:shakacode/pack-config-diff resolves to whatever the default branch HEAD is at install time. Yarn.lock currently freezes it to commit 6bb09f4ba933ef5978c534b1c278156e438dce2e, which protects yarn users today, but yarn upgrade or a fresh npm install (npm ignores yarn.lock) will silently pull new code. A tag or commit pin would make this explicit and reproducible.

2. The npx fallback in bin/diff-bundler-config: npx -y github:shakacode/pack-config-diff ... always fetches the current HEAD. This runs entirely outside the lock file, so any push to that repo is immediately executed by users of this fallback path. This is a supply chain risk for a CLI tool shipped with every Shakapacker install. Fix: pin to the same commit hash already in yarn.lock, or remove the fallback entirely until the package is published to npm (the PR description notes this as a near-term goal) and print a helpful error if the local module is not found.

Code quality: confusing control flow in the catch block (minor)

Calling runViaNpx() always ends with process.exit(), so the throw error that follows in the same catch block is unreachable in practice. But the code looks like it re-throws for both MODULE_NOT_FOUND and non-MODULE_NOT_FOUND cases. Adding a return after runViaNpx() makes the intent clear. See inline comment.

Potential silent failure when run is not a function (minor)

If require('pack-config-diff') succeeds but the module does not export a run function, run is undefined. The second try/catch prints "run is not a function" which is not actionable for users. A guard like if (typeof run !== 'function') with a descriptive error would be better.

Duplication

bin/diff-bundler-config and lib/install/bin/diff-bundler-config are byte-for-byte identical, consistent with how bin/shakapacker-config is managed, but future changes must be applied in two places. A comment noting the sibling file would help maintainers.

Verdict

The core design is good. The blocking item is the unpinned npx fallback — everything else is refinements.

Greptile Summary

This PR introduces semantic bundler config diffing to Shakapacker by delegating to the extracted pack-config-diff package instead of embedding a diff engine in the repo. It adds a bin/diff-bundler-config binstub (with a lib/install/bin/ counterpart for gem installation), registers it as an expected binstub in the doctor checks, updates README and docs to use it, and adds a new guide at docs/config-diff.md.

Key changes and findings:

• New binstub (bin/diff-bundler-config, lib/install/bin/diff-bundler-config): tries a local require("pack-config-diff") first, then falls back to npx -y github:shakacode/pack-config-diff. The error handling is slightly inconsistent — non-MODULE_NOT_FOUND require errors escape to an unhandled exception rather than using the clean console.error + process.exit(1) pattern, and there's no guard if the package loads but doesn't export run.
• Unpinned GitHub dependency (package.json): "pack-config-diff": "github:shakacode/pack-config-diff" carries no tag, semver range, or commit hash. The lockfile currently pins to a specific commit, but a lockfile regeneration would pull the latest HEAD, which could silently introduce breaking changes for a production dependency.
• Doctor & specs: the binstub presence checks and test coverage are well-implemented and consistent with existing patterns.
• Documentation: the new docs/config-diff.md guide is thorough and the README/troubleshooting/migration guide updates are clean.

Confidence Score: 3/5

• Safe to merge after addressing the unpinned GitHub dependency; the binstub logic issues are low-risk but worth fixing before a release.
• The doctor/spec changes and documentation are solid. The main concerns are the unpinned github: dependency in package.json (non-reproducible after lockfile regeneration) and the inconsistent error handling in the binstub for edge-case failures.
• package.json (unpinned dependency) and bin/diff-bundler-config / lib/install/bin/diff-bundler-config (error handling edge cases).

Important Files Changed

Sequence Diagram

sequenceDiagram
    participant User
    participant Binstub as bin/diff-bundler-config
    participant LocalPkg as pack-config-diff (local)
    participant NPX as npx (github:shakacode/pack-config-diff)

    User->>Binstub: node bin/diff-bundler-config --left=a.yaml --right=b.yaml

    Binstub->>LocalPkg: require("pack-config-diff")

    alt Module found locally
        LocalPkg-->>Binstub: { run }
        Binstub->>LocalPkg: run(argv)
        LocalPkg-->>Binstub: exitCode
        Binstub-->>User: process.exit(exitCode)
    else MODULE_NOT_FOUND
        Binstub->>NPX: spawnSync("npx", ["-y", "github:shakacode/pack-config-diff", ...argv])
        NPX-->>Binstub: { status, error }
        alt npx succeeded
            Binstub-->>User: process.exit(status ?? 1)
        else npx error
            Binstub-->>User: console.error + process.exit(1)
        end
    else Other require error
        Binstub-->>User: throw error (unhandled exception)
    end

_{Last reviewed commit: e022a56}

Review

Good idea to extract the diff engine into a separate package. The docs and Ruby-side changes are clean. A few issues need addressing before merge.

Blocking

1. pack-config-diff is not actually added to package.json. The PR description claims the package was added as a dependency, but package.json was not modified. The require() will always fail and fall back to npx on every run, defeating the local-first pattern. Either add it as an optional dependency or correct the PR description.

2. npx fallback will hang in CI. runViaNpx() uses spawnSync without -y. In non-interactive/CI environments npx will prompt for consent and either hang or fail. Add -y or gate it on process.env.CI if CI use is intended.

Non-blocking

3. Fragile control flow. runViaNpx() uses process.exit() as its return mechanism. The code after the catch block is unreachable when the fallback fires, but this will silently break if the function is ever refactored to return normally. Worth annotating or restructuring.

4. lib/install/bin/diff-bundler-config is missing from npm files. The package.json files array only includes package and lib/install/config/shakapacker.yml. This binstub template will not be included in the published npm package. Add lib/install/bin to files if dual distribution is needed.

5. Ambiguous exit code in docs. docs/config-diff.md documents exit code 1 as meaning both differences found and error occurred. This makes reliable CI scripting impossible. If pack-config-diff supports distinct exit codes, update the docs; otherwise document the limitation.

Review: PR 973 — Add bin/diff-bundler-config wrapper

Summary

The overall approach is sound — delegating config diffing to an extracted package keeps Shakapacker focused. A few issues need attention before merging.

Must Fix

1. Exit code divergence between bin/diff-bundler-config and lib/install/bin/diff-bundler-config

The two files are meant to be identical, but the checked-in bin/diff-bundler-config uses process.exit(1) for all error paths, while lib/install/bin/diff-bundler-config (the installer template) correctly uses process.exit(2) for runtime errors.

docs/config-diff.md documents:

• 0 — no differences
• 1 — differences found
• 2 — wrapper/runtime error

Using exit(1) for errors makes crashes indistinguishable from "configs differ". Any CI step checking $? -eq 1 to detect config differences will silently misinterpret a tool crash. All runtime error process.exit(1) calls in bin/diff-bundler-config should be process.exit(2). (See inline comment at line 27.)

2. pack-config-diff is missing from package.json

The PR description says "Added pack-config-diff dependency in package.json" but the package.json diff is empty — the package is absent from all dependency sections. Users have no declarative way to install it; the only discovery path is running the binstub and watching it fall back to npx.

Recommended: add it as an optional peer dependency with a corresponding peerDependenciesMeta entry (marked optional), so package managers surface it without requiring it.

Should Fix

3. Doctor warns all existing users about an optional tool

Every existing Shakapacker project without bin/diff-bundler-config will get a warning on every doctor run. Since this is explicitly optional, the warning creates noise for users who have no interest in config diffing. Consider emitting add_info instead of add_warning, or only warning when pack-config-diff is already present in node_modules. (See inline comment on lib/shakapacker/doctor.rb.)

4. CI detection is too narrow

The check process.env.CI === "true" || process.env.GITHUB_ACTIONS === "true" misses CI platforms that set CI=1 or other non-"true" values. Using a truthy check on process.env.CI is the standard convention and covers more platforms. (See inline comment on bin/diff-bundler-config:35.)

Minor

5. return runViaNpx() at module top-level

Works in Node.js CommonJS (files are wrapped in an implicit function), but it is an unusual pattern that may confuse contributors. Wrapping top-level logic in an explicit main() call would be cleaner. (See inline comment on line 72.)

Overall this is a clean approach - delegating the diff engine to an extracted package keeps Shakapacker focused, and the wrapper script is well-structured. Two issues worth fixing before merge.

Bug: GEM_ROOT constant collision in binstub_sync_spec.rb

generator_spec.rb already defines a top-level constant GEM_ROOT = Pathname.new(...). The new spec defines GEM_ROOT = File.expand_path(...) inside a describe block, but Ruby hoists that constant to Object, causing an "already initialized constant" warning and a type mismatch (String vs Pathname) depending on load order when running the full suite.

Fix: use a local variable (gem_root =) in binstub_sync_spec.rb and reference it in the Dir.glob/File.read calls below (see inline comment).

Minor: implicit fallthrough after runViaNpx()

runViaNpx() always calls process.exit() on every code path so execution never returns to the caller, but there is no return following the call. A future change adding a non-exit code path to runViaNpx would silently fall through with run === undefined. Adding return after the call makes the intent clear (see inline comment). The same applies identically to lib/install/bin/diff-bundler-config.

Notes (not blocking)

• Duplicate binstub maintenance: binstub_sync_spec.rb guards against drift between bin/ and lib/install/bin/, which is good. A comment at the top of each file pointing to its counterpart would further reduce the chance of them diverging.
• npx version range: getNpxSpec() can return pack-config-diff@^0.1.0 (a semver range). npm/npx handles this correctly in all versions likely in use; just noting it.
• docs/config-diff.md shows --ignore-paths="plugins.*" but whether * is glob or regex syntax depends on pack-config-diff internals - a pointer to the upstream docs for exact syntax would be helpful.

Review feedback addressed (3babdee)

Triaged 3 unresolved review threads. Fixed 1 item, posted rationale replies on 2 skipped items, and resolved all threads.

Changes made

1. Fix GEM_ROOT constant collision (spec/shakapacker/binstub_sync_spec.rb) — Changed GEM_ROOT constant to gem_root local variable to avoid collision with generator_spec.rb (Pathname vs String type mismatch when running the full test suite).

Skipped with rationale

2. Implicit fallthrough after runViaNpx() (bin/diff-bundler-config:88) — runViaNpx() unconditionally exits on every path; the suggested return would be dead code.
3. Negative exit codes bypass normalization (bin/diff-bundler-config:45) — exitWithCode only receives values from spawnSync().status (0–255 or null) and run() callback (0 or 1); negative integers aren't practically possible.

Review: Supersede #961 with pack-config-diff

The approach of delegating the diff engine to an extracted package is the right call — it keeps Shakapacker focused and puts maintenance of the diff logic in one place. The required/optional binstub split is clean and the binstub sync spec is a smart safeguard.

Three issues worth addressing (inline comments posted):

1. Missing -- separator in npx call (line 61)
process.argv.slice(2) is spread directly after the package spec, so any user flag that matches an npx built-in (e.g. --yes, --no, --quiet) gets consumed by npx rather than forwarded to pack-config-diff. Adding "--" as the separator fixes this.

2. getNpxSpec() version lookup is a no-op in installed projects (lines 8–19)
When the binstub is copied into a user's project from lib/install/bin/, __dirname resolves to the user's bin/, so the package.json lookup reads the user's project manifest. pack-config-diff is declared in Shakapacker's optionalDependencies, not the user's, so the lookup always falls through to ^0.1.0. Not a bug (the fallback is fine), but a short comment explaining this would help future maintainers.

3. Binstub sync spec excludes the case it should catch (lines 9–11)
The current guard generates a test only for binstubs that already exist in both locations. If a future lib/install/bin/ entry is added without a matching bin/ copy, the spec generates zero examples and the suite passes silently. Inverting the filter so every lib/install/bin/ entry generates a test (and fails if bin/ is missing) would make the spec actually enforce the invariant.

Review feedback addressed (0ce0f32)

Triaged 3 new review comments. Addressed all by promoting pack-config-diff from optionalDependencies to dependencies and removing the entire npx fallback path.

Changes made

1. Moved pack-config-diff to regular dependencies in package.json — it's small, so always installing it is simpler than the npx fallback machinery.
2. Removed ~60 lines per binstub — getNpxSpec(), runViaNpx(), isMissingPackConfigDiff(), spawnSync import, and the CI guard are all gone from both bin/diff-bundler-config and lib/install/bin/diff-bundler-config.
3. Updated docs — removed "npx fallback failure" from exit code description.

Review comments resolved

• Missing -- separator in npx call — resolved by removing npx entirely
• getNpxSpec() is a no-op in user projects — resolved by removing npx entirely
• Binstub sync spec excludes missing entries — declined; the spec's purpose is to catch divergence between the two copies, not enforce that every install template has a repo counterpart

Review: Supersede #961 by using pack-config-diff

Overall the approach is clean — delegating to an extracted package keeps Shakapacker focused and the wrapper script's defensive exit-code handling is a nice touch. A few issues worth addressing before merge:

1. PR description contradicts the implementation (no npx fallback)

The PR description states:

If unavailable in the local install layout, it falls back to npx -y github:shakacode/pack-config-diff ... so the command remains usable before npm publication.

But the actual bin/diff-bundler-config has no npx fallback — if require("pack-config-diff") fails it simply exits with code 2. Either the description should be updated to reflect the real behavior, or the intended fallback should be implemented. Users who run the binstub without the package installed will get an opaque error rather than a helpful auto-fetch.

2. Supply chain / trust surface for a new 0.1.0 package

Adding a runtime dependency on a brand-new 0.1.0 package (pack-config-diff) introduces non-trivial supply chain risk for all Shakapacker users. A few considerations:

• The yarn.lock pins the exact version (0.1.0) and hash, which is good.
• However, the ^0.1.0 range in package.json means any future 0.1.x patch release (including a potentially compromised one) will be auto-installed by users without a lockfile.
• The package depends on js-yaml, which Shakapacker already depends on — the transitive graph is minimal, so the blast radius is contained.

Recommendation: pin to an exact version ("pack-config-diff": "0.1.0") until the package has more release history, or at least until its API has stabilized.

3. pack-config-diff belongs in optionalDependencies, not dependencies

This is purely a debugging/developer-experience tool. Putting it in dependencies means every Shakapacker user installs it unconditionally, including in production CI environments that use --omit=optional or equivalent flags to keep installs lean. The binstub already handles the missing-module case gracefully, so making it optional costs nothing functionally. (Inline comment on package.json.)

4. lib/install/bin/diff-bundler-config is not in the npm files list

package.json files is ["package", "lib/install/config/shakapacker.yml"]. The lib/install/bin/ directory is not included, so diff-bundler-config will not ship with the npm package. This is likely intentional (distributed via the gem only), but worth a comment to make the decision explicit and prevent someone from thinking it was accidentally omitted.

Minor issues (inline comments posted)

• Negative exit codes pass Number.isInteger and satisfy exitCode <= 1, so they would leak through the normalization in exitWithCode. Use exitCode === 0 || exitCode === 1 instead.
• run() returning null/undefined produces a confusing "non-integer exit code: null" error rather than explaining that the function returned nothing.
• Double yarn install in Rakefile and test-bundlers.yml is correct but needs a comment explaining the pattern (first install for base deps, then yalc add rewrites package.json, second install picks up the local build).
• print_binstub_status silent empty state — when zero required binstubs are found, the output section prints nothing (though the check still adds a warning). Adding an else branch keeps the report visually consistent.

Review feedback addressed (eef1c25)

Triaged 6 unresolved review threads. Fixed 1 item, posted rationale replies on 5 skipped items, and resolved all threads.

Changes made

1. Fix strict package manager resolution (eef1c25): Both binstubs now use createRequire(require.resolve("shakapacker/package.json")) to resolve pack-config-diff via shakapacker's dependency tree instead of the app's. This fixes the binstub under pnpm isolated node_modules and Yarn PnP where transitive dependencies are not resolvable from the app's bin/ directory.

Declined with rationale

• Negative exit code guard (exitCode <= 1) — pack-config-diff only returns 0/1
• null/undefined return from run() — already handled correctly (exits 2)
• Move to optionalDependencies — intentionally in dependencies per 0ce0f32
• Rakefile comments — standard yalc pattern, self-documenting
• doctor.rb else branch — check_binstub already warns for missing binstubs

Review Summary

Overall this is a clean approach — delegating diffing to a dedicated package keeps Shakapacker focused, and the doctor/binstub refactor (extracting REQUIRED_BINSTUBS/OPTIONAL_BINSTUBS constants) is a nice improvement. Three issues worth addressing before merge:

pack-config-diff declared in dependencies, not optionalDependencies

The PR frames diff-bundler-config as optional throughout (doctor treats it as optional, binstub has load-failure handling), but package.json puts it in dependencies — meaning it installs for every user unconditionally. This contradicts the "optional" framing and makes the binstub's try/catch dead code in practice. It should move to optionalDependencies.

PR description mentions an npx fallback that isn't in the code

The summary says the binstub "falls back to npx -y github:shakacode/pack-config-diff" when the local module is unavailable, but the actual implementation just exits with code 2. Either the fallback was intentionally dropped (update the description) or it was accidentally omitted (add it back for the pre-npm-publication use case).

binstub_sync_spec.rb generates zero examples silently if glob finds nothing

shared_binstubs.each runs at describe-time, so if lib/install/bin/ ever becomes empty or gets renamed, the spec generates no examples and the suite passes silently. A guard (raise or an explicit presence check) would prevent false-green runs.

Everything else looks good

• The binstub's exit-code normalization (0/1/2) is correct and well-documented
• Handling both sync and async run() return values is a nice touch
• yalc add + double yarn install in CI/Rakefile is the right fix for yalc link's limitations
• The binstub_sync_spec idea itself is excellent — catching drift between bin/ and lib/install/bin/ is exactly the kind of check that prevents subtle bugs