Fix Sidekiq 7+ and the install migration (3.6.3)

[schlick]

Bug fixes plus test and tooling work, deliberately scoped so that anything which
installed 3.6.2 installs this. git diff develop -- patches.gemspec is comments
only: no runtime constraint changes, no API changes, nothing removed from the
public interface.

One qualification, recorded under Known issues in the CHANGELOG: because the
install migration now declares ActiveRecord::Migration[5.0], a fresh
install on Rails 4.2 or earlier needs patches_patches created by hand. The
file it replaces could not be loaded by any Rails from 5.0 on, so this trades an
install that was impossible on every version CI verifies for one that is
unlikely. Existing installs copied the migration into the host application long
ago and are unaffected.

First step of a staged modernisation — declaring activerecord, bounding
slack-notifier and adding deprecations belong in 3.7.0; raising minimums in
4.0.0.

Fixes

Sidekiq 7+ rejected our job arguments. Strict argument checking, on by
default from Sidekiq 7, refused what the workers enqueued: patches:run passed
the runner class object and application_version used a symbol key, so
enqueueing raised

ArgumentError: Job arguments to Patches::Worker must be native JSON types,
but Patches::Runner is a Class

The task now enqueues runner.name and the key is a string. The serialised
payload is byte-identical to 3.6.2's — verified by putting both call styles
through Sidekiq's JSON encoding:

old payload: ["Patches::Runner",{"application_version":"abc"}]
new payload: ["Patches::Runner",{"application_version":"abc"}]
identical on the wire: true

The class already reached Redis as "Patches::Runner" and the key as
"application_version", so in-flight jobs and rolling deploys mixing old and
new workers are unaffected. The workers also normalise params keys to strings so
the version-mismatch retry can't re-enqueue symbol keys.

Older Sidekiq keeps working. Rather than switching to Sidekiq::Job
outright, the workers resolve their mixin through Patches.sidekiq_job_module,
which prefers Sidekiq::Job (the name Sidekiq has used since 6.3) and falls
back to Sidekiq::Worker. A spec covers both branches.

The install migration never worked on modern Rails. It inherited from a bare
ActiveRecord::Migration, which Rails has refused since 5.0 with "Directly
inheriting from ActiveRecord::Migration is not supported", so
rake patches:install:migrations && rake db:migrate failed on every Rails since
— the table must have been created by hand. It now declares
ActiveRecord::Migration[5.0]. That version deliberately: Rails still ships the
V4_2-onward compatibility shims, confirmed on 6.1, 7.2 and 8.1, so the fix
widens compatibility without raising a floor. spec/migration_spec.rb
covers a file that shipped with no coverage at all.

Patches.default_path guarded on defined?(:Rails), which tests a symbol
literal and is always truthy, so outside Rails it raised NameError instead of
returning nil.

Removed

lib/generators/patches.rb: a dead duplicate of the patch generator, writing to
app/db/ from a template that does not exist. Rails' generator lookup only
loads *_generator.rb, so it was unreachable and not public interface.

CI

One Ruby 2.7 cell becomes 14 Ruby/Rails combinations (Ruby 3.0–3.4 against Rails
7.1–8.1), plus a leg per Sidekiq state: absent, 6.5, and 7.x/8.x with strict
arguments both on and off.

Ruby 2.7 and Rails 6.1/7.0 are left out as long past upstream support. Nothing
blocks them — no constraint moved — and README.md documents how to check such a
combination locally.

Development dependencies are pruned to those actually used (six were referenced
by no spec, and rspec-rails was never required), and rails, sqlite3, sidekiq
and concurrent-ruby move to the Gemfile. CI varies rails and sidekiq through it;
the sqlite3 and concurrent-ruby knobs are for reproducing a pre-7.1 Rails
locally, which no cell now covers.

The stale sidekiq ~> 3.4.1 pin is what hid this incompatibility from CI, and
it also held json at ~> 1.0. Dropping it closes the three open Dependabot
alerts (1 high, 2 moderate), all of which are that pin; Sidekiq was only ever
a development dependency, so no consumer was exposed through Patches.

The SonarQube analysis moves into its own job rather than trailing the spec run,
so coverage is now measured on Ruby 3.2 instead of 2.7. SimpleCov is untouched
here and the report path stays coverage/.resultset.json; replacing that with
the JSON formatter is a separate change.

Verification

All 14 cells and all 6 Sidekiq legs green — 63 examples, 48 with Sidekiq absent
— so every combination this release claims has been run. Rails 6.1 and 7.0 were
also run once on Ruby 3.0, green, to support the statement that they still work
while going untested.

CI found a bug that the local sweep did not. The first run failed 6 of 22
checks with ActiveModel::MissingAttributeError in spec/patch_spec.rb. The
scattered pattern — Ruby 3.2/7.1 passing while 3.1/7.1 failed, Sidekiq 8.0
failing only with strict arguments off — was order-dependence rather than a
version problem: the new migration_spec dropped and recreated the shared
patches_patches table, leaving Patches::Patch holding column information for
a table that no longer existed, so whichever spec the random seed ran next blew
up.

The local sweep missed it because every one of those cells reused a test.db
left from an earlier run, while CI always creates it fresh — and only on that
path does the drop corrupt the model's column cache. Deleting test.db
reproduced it immediately.

a604482 fixes it by running the migration against a throwaway in-memory
database, so the shared one is never touched, restoring the connection and
resetting column information afterwards. Verified across 10 seeds with a fresh
database each time, plus a direct before/after on the seed that failed:

before the fix, seed 37006: 62 examples, 2 failures
after the fix,  seed 37006: 62 examples, 0 failures

Worth carrying forward: rm -f test.db belongs in the local verification
recipe, and the suite would be better off not depending on a persistent sqlite
file at all — database_cleaner sat in the gemspec for years without ever being
wired up. A candidate for the 3.7.0 tooling work.

Review round

Copilot raised four inline comments; each is answered in its thread.

• transform_keys excluded older stacks (worker.rb, tenant_worker.rb) —
  valid. Fixed in 8097dbc: ActiveSupport's stringify_keys predates both
  Hash#transform_keys (Ruby 2.5) and ActiveSupport's own (Rails 4.0), so it
  excludes nothing the gemspec permits. Also added an explicit
  require "active_support/core_ext/hash/keys" rather than relying on the host
  application having loaded it.
• A Pathname path is rejected by strict argument checking
  (tenant_runner.rb) — valid. Fixed in a3ac178 by passing
  path && path.to_s, with a spec that fails without the change. Unreachable
  through documented usage — the rake task always calls runner.new with no
  arguments — but TenantRunner.new(path:) is a public constructor.
• The versioned migration breaks Rails 3.2/4.2 — valid, and deliberately
  documented rather than fixed (b5d02ca). A conditional superclass was
  considered and rejected: it would bend the shipped migration around versions
  no test exercises, and Rails 3.2/4.2 cannot run on any Ruby this project
  builds against, so the branch would be permanently unverified. Raising the
  railties floor to match reality is queued for 4.0.0.

Reviewing

Eleven commits, one idea each; the three bug fixes are separate. Note that the
first five carry the old gemspec pins, since the tooling change lands in
cde1f1d — they're runnable on Ruby 2.7 but not on a modern Ruby, where Sidekiq
3.x won't install. a604482 fixes the CI failure described above, and the last three
commits are the review responses — all kept as their own commits rather than
amends, since the branch was already under review.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Three critical compatibility regressions and one moderate Sidekiq argument issue remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes Sidekiq 7+ compatibility and Rails migration behavior for release 3.6.3 while expanding tests and CI coverage.

Changes:

• Adds JSON-safe Sidekiq arguments and version-compatible mixins.
• Fixes migration and non-Rails path handling.
• Modernizes testing, dependencies, documentation, and CI.

File summaries

File	Review
spec/tenant_runner_spec.rb	Updates inline tenant-runner coverage.
spec/spec_helper.rb	Configures strict Sidekiq arguments.
spec/sidekiq/worker_spec.rb	Tests strict-argument enqueueing.
spec/sidekiq/tenant_worker_spec.rb	Updates retry expectations.
spec/sidekiq/tenant_runner_spec.rb	Tests parallel tenant enqueueing.
spec/sidekiq/sidekiq_job_spec.rb	Tests Sidekiq mixin selection.
spec/sidekiq_optional_spec.rb	Tests optional Sidekiq integration.
spec/patches_spec.rb	Tests operation without Rails.
spec/migration_spec.rb	Tests migration loading and execution.
README.md	Documents compatibility and local testing.
patches.gemspec	Prunes development dependencies.
lib/tasks/patches.rake	Enqueues JSON-native runner data.
lib/patches/worker.rb	Critical (2 votes): transform_keys breaks supported older Ruby/Rails stacks.
lib/patches/version.rb	Bumps the version to 3.6.3.
lib/patches/tenant_worker.rb	Critical (2 votes): transform_keys breaks supported older Ruby/Rails stacks.
lib/patches/tenant_runner.rb	Moderate (1 vote): explicit Pathname values remain incompatible with strict Sidekiq arguments.
lib/patches/sidekiq_job.rb	Selects the available Sidekiq mixin.
lib/patches.rb	Fixes Rails detection and conditional Sidekiq loading.
lib/generators/patches.rb	Removes an obsolete generator.
Gemfile	Adds environment-selectable test dependencies.
db/migrate/201506011700_create_patch.rb	Critical (2 votes): versioned migration inheritance fails on still-supported Rails 3.2/4.2.
CHANGELOG.md	Documents release 3.6.3.
.github/workflows/specs.yml	Expands Rails, Ruby, and Sidekiq CI coverage.

Review details

• Files reviewed: 23/23 changed files
• Comments generated: 4
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

The migration breaks fresh installs on still-permitted Rails 3.2/4.2 versions and needs a compatible superclass fallback.

Review details

Suppressed comments (3)

README.md:66

• These environment assignments apply only to bundle install (including the assignment before the && command), not to the subsequent bundle exec. The absent recipe therefore reevaluates the Gemfile with Sidekiq enabled and can reject the lockfile, while the strict-off recipe never sets SIDEKIQ_STRICT_ARGS=false during the specs. Repeat the variables for the test command or export them first.

SIDEKIQ_VERSION=none bundle install
bundle exec rspec --exclude-pattern "sidekiq/**/*_spec.rb"

SIDEKIQ_VERSION="~> 7.0" SIDEKIQ_STRICT_ARGS=false bundle install && bundle exec rspec

README.md:75

• The pins here are scoped only to bundle install; the next command reevaluates the Gemfile with its default rails >= 7.1, so Bundler sees a changed dependency and the Rails 6.1 recipe cannot run as written. Apply the same variables to bundle exec rspec (or export them).

RAILS_VERSION="~> 6.1.0" SQLITE3_VERSION="~> 1.4" CONCURRENT_RUBY_VERSION="< 1.3.5" bundle install
bundle exec rspec

db/migrate/201506011700_create_patch.rb:1

• This creates an effective Rails 5 floor for fresh installs even though the gemspec still permits Rails 3.2+ and the release notes say older Rails are not blocked. Rails 3.2/4.2 do not implement ActiveRecord::Migration.[], so loading the copied migration now fails before creating the table. Keep the versioned parent on modern Rails while falling back to the former parent where the version API is unavailable.

class CreatePatch < ActiveRecord::Migration[5.0]

• Files reviewed: 23/23 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[schlick]

Reviewed the three suppressed comments from the latest pass. Two were correct and are fixed in af99c0c; the third is a restatement of a decision recorded above.

README.md:66 and README.md:75 — both valid, and worse than they look. The variables were prefixed only to bundle install, so bundle exec re-resolved the Gemfile with its defaults. Verified before fixing:

• the "Sidekiq absent" recipe ran with sidekiq 8.1.7 loadable — it was skipping the spec files, not testing the absent path
• the Rails 6.1 recipe produced Rails 8.1.3.1
• the strict-args recipe never set SIDEKIQ_STRICT_ARGS for the specs at all

Each recipe now uses an export block covering both commands, with a note that bundler re-evaluates the Gemfile on every invocation. All four were then run verbatim from a clean shell and confirmed to do what they claim:

Recipe	Result
RAILS_VERSION="~> 8.0.0"	Rails 8.0.5.1 — 63 examples, 0 failures
SIDEKIQ_VERSION=none	sidekiq absent — 48 examples, 0 failures
SIDEKIQ_VERSION="~> 7.0" SIDEKIQ_STRICT_ARGS=false	sidekiq 7.3.9 — 63 examples, 0 failures
Rails 6.1 with both pins	Rails 6.1.7.10, sqlite3 1.7.3 — 63 examples, 0 failures

CI was never affected — job-level env applies to every step — and the verification sweeps exported the variables, so their results stand. The flaw was confined to the README, where it would have misled a reader.

I also added a line about deleting test.db between combinations, since a database left by an earlier run masks ordering problems — which is exactly how the failure in a604482 reached CI.

db/migrate/201506011700_create_patch.rb:1 — same point as the earlier inline comment, answered in that thread. The decision stands: keep ActiveRecord::Migration[5.0] and document the limitation, rather than add a respond_to?(:[]) fallback that Rails 3.2/4.2 would be needed to exercise and which no Ruby this project builds against can run. The CHANGELOG has a Known issues entry and the PR description is qualified accordingly. Raising the railties floor to match what is actually tested is queued for 4.0.0.

[Copilot]

🟢 Approval recommended

The reviewed changes have no unresolved approval-blocking issues.

Review details

• Files reviewed: 23/23 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[sonarqube-rdytech]

Quality Gate passed

Issues

• 0 New Issues
• 0 Fixed Issues
• 0 Accepted Issues

Measures

• 0 Security Hotspots
• 93.30% Coverage (90.70% Estimated after merge)
• 0.00% Duplicated Code (0.00% Estimated after merge)

Project ID: rdytech_patches_4fe7fcf4-bbfc-4b19-be78-87e73d1f301e

View in SonarQube

[cyanpereyra-readytech]

LGTM +1 🚀