WA-RAILS7-020: ActiveRecord attribute API audit

[kitcommerce]

Fixes #751.

What changed

• Audited the codebase for ActiveRecord::Type usage (none found).
• Added a Rails 7.0 compatibility Gemfile for CI coverage.
• Hardened boot against Rails 7-era dependency changes:
  • Guard Redis.sadd_returns_boolean= for redis-rb 5+
  • Ensure Sidekiq::Callbacks is loaded before config validation

Verification

• rg ActiveRecord::Type → no matches
• BUNDLE_GEMFILE=gemfiles/rails_7_0.gemfile bundle install

Client impact

None expected.

[kitcommerce]

Simplicity Review

Verdict: ✅ APPROVED — No simplicity concerns

Reviewed by: Kit (simplicity-reviewer) | Wave 1

---

This PR is a textbook example of proportional complexity. Four small changes, each solving exactly what's needed:

• engine.rb — Redis.respond_to?(:sadd_returns_boolean=) is the canonical Ruby idiom for version-safe method calls. No abstraction, no indirection.
• sidekiq.rb — require ... unless defined?(...) guard is idiomatic and minimal. One line solves the load-order issue.
• rails_7_0.gemfile — 5 lines. Inherits the parent Gemfile, pins one gem. Could not be simpler.
• ci.yml — 3 lines to add a CI matrix entry. Perfectly proportional.

No new abstractions, no protocols, no factories, no generics, no premature generalization. The scaffolding is exactly proportional to the need.

Nothing to flag here.

[kitcommerce]

Rails Conventions Review

{
  "reviewer": "rails-conventions",
  "verdict": "PASS",
  "severity": null,
  "summary": "No Rails convention violations found — diff is scoped to CI matrix config, defensive boot guards, and a multi-Gemfile pin.",
  "findings": []
}

This PR makes no changes to controllers, models, routes, scopes, callbacks, service objects, or ActiveRecord queries — the areas this reviewer evaluates. The changes present are:

• **** — CI matrix entry for Rails 7.0 (experimental). Infrastructure only.
• **** — guard on . This is the idiomatic Ruby way to handle an API that may or may not exist depending on gem version.
• **** — . Standard Rails auto-load guard; correct placement in engine configuration.
• **** — + pinned Rails 7.0.10. Textbook multi-Gemfile pattern for version matrix CI.

All patterns here work with the framework grain rather than against it. Nothing to flag.

[kitcommerce]

🔒 Security Review

{
  "reviewer": "security",
  "verdict": "PASS",
  "severity": null,
  "summary": "No security concerns — changes are limited to CI configuration, boot-order guards, and a new gemfile with no attack surface.",
  "findings": []
}

Scope reviewed: CI matrix addition, respond_to? guard on Redis config setter, conditional require for Sidekiq::Callbacks, and eval_gemfile-based Rails 7.0 gemfile. No secrets, auth logic, input handling, cryptography, or data exposure paths are touched.

[kitcommerce]

🏗️ Architecture Review

{
  "reviewer": "architecture",
  "verdict": "PASS",
  "severity": null,
  "summary": "Minimal defensive changes that respect existing module boundaries and introduce no new coupling.",
  "findings": []
}

Notes:

• CI matrix addition (ci.yml, rails_7_0.gemfile): Follows the established gemfile-per-Rails-version pattern. eval_gemfile delegation to the root Gemfile keeps dependency declaration centralized. No concerns.
• Redis guard (engine.rb): respond_to? check is a standard cross-version compatibility idiom. Keeps the engine boot path resilient without introducing any new abstractions or dependencies.
• Sidekiq::Callbacks require guard (sidekiq.rb): Defensive require before assert_valid_config! ensures correct load ordering. This stays within the existing configuration module — no layer violation, no new coupling.

All changes are localized, defensive, and consistent with the existing architecture. No new abstractions, no coupling introduced, no boundary violations.

[kitcommerce]

{
  "reviewer": "rails-security",
  "verdict": "PASS",
  "findings": []
}

Rails Security Review — PASS ✅

No security-relevant changes in this diff. The PR contains:

1. CI matrix addition (ci.yml) — adds a Rails 7.0 test target marked experimental: true. No code execution path change.
2. respond_to? guard on Redis.sadd_returns_boolean= (engine.rb) — defensive compatibility check. No new attack surface; the setter controlled an internal Redis client behavior flag.
3. Defensive require for Sidekiq::Callbacks (sidekiq.rb) — ensures the constant is defined before calling assert_valid_config!. No security implications.
4. New Gemfile (rails_7_0.gemfile) — pins rails 7.0.10 via eval_gemfile. Standard pattern, no dependency confusion risk (uses parent Gemfile as base).

All 10 security checklist items (strong params, SQL injection, XSS, CSRF, IDOR, auth/authz, secrets, redirects, file uploads, record scoping) are not applicable — none of these areas are touched by this change.

[kitcommerce]

🗄️ Database Review — PASS

{
  "reviewer": "database",
  "verdict": "PASS",
  "findings": []
}

Summary: No database concerns. This PR contains:

• CI workflow addition (Rails 7.0 matrix entry)
• A defensive respond_to? guard on Redis.sadd_returns_boolean=
• A require guard for Sidekiq::Callbacks
• A new Gemfile for Rails 7.0

No migrations, schema changes, query modifications, model changes, or data integrity implications.

[kitcommerce]

{
  "reviewer": "test-quality",
  "verdict": "PASS_WITH_NOTES",
  "severity": null,
  "summary": "No tests added, but the changes are boot-time guards and CI infrastructure — appropriate coverage comes from the Rails 7.0 matrix entry running the existing test suite.",
  "findings": [
    {
      "severity": "LOW",
      "file": "gemfiles/rails_7_0.gemfile",
      "line": 1,
      "issue": "The audit finding (no ActiveRecord::Type usage) has no regression guard. If someone introduces ActiveRecord::Type usage in the future, nothing in the test suite will flag it as a compatibility concern for Rails 7+.",
      "suggestion": "Consider adding a lint-level test (e.g., a grep-based test in a CI step or a Ruby test file) that asserts zero matches for `ActiveRecord::Type` usage in application code, locking in the audit result as a permanent gate."
    }
  ]
}

Test Quality Review — PASS_WITH_NOTES

The three code changes in this PR are all boot-time initializer guards:

• Redis.respond_to?(:sadd_returns_boolean=) guard in engine.rb
• require 'sidekiq/callbacks' unless defined?(::Sidekiq::Callbacks) in sidekiq.rb

These run during Rails engine boot — not during request handling or business logic execution. Unit-testing boot-time require/respond-to guards in Rails is uncommon and provides minimal value; the correct verification vehicle is running the full test suite on the target Rails version, which is exactly what the new CI matrix entry does.

The Rails 7.0 acceptance criterion ("dummy apps boot clean on Rails 7") is adequately addressed by the experimental: true matrix entry running existing tests against Rails 7.0. This is standard practice for Rails version compatibility coverage.

The one observation worth tracking: The audit conclusion — "zero ActiveRecord::Type usage in this codebase" — is a snapshot finding with no enforcement. It will silently rot if the codebase grows. A simple CI grep step (or even a one-liner test file) could lock this in permanently as a regression gate. Low priority, but it would complete the audit story.

[kitcommerce]

Performance Review

{
  "reviewer": "performance",
  "verdict": "PASS",
  "severity": null,
  "summary": "No performance concerns. All changes are boot-time initialization guards and CI configuration with negligible runtime impact.",
  "findings": []
}

Analysis:

• engine.rb — Added Redis.respond_to?(:sadd_returns_boolean=) guard: O(1) method-existence check executed once at boot. Zero runtime overhead.
• sidekiq.rb — Added require ... unless defined?(::Sidekiq::Callbacks) guard: defined? is a compile-time constant check, effectively free. Executes once during initialization.
• gemfiles/rails_7_0.gemfile — CI-only gemfile. No production runtime impact.
• ci.yml — Adds a Rails 7.0 matrix entry (experimental). CI wall-clock time increases by ~1 parallel job, but this is acceptable CI hygiene cost with no application performance implications.

No hot paths touched. No allocations, I/O, queries, or algorithmic changes introduced. Clean boot-hardening with no performance tradeoffs.

[kitcommerce]

Frontend Review

{
  "reviewer": "frontend",
  "verdict": "PASS",
  "severity": null,
  "summary": "No frontend changes in this diff. All modifications are backend/infrastructure: CI workflow matrix addition, Redis sadd_returns_boolean guard, Sidekiq::Callbacks eager-require, and a new Rails 7.0 gemfile. No JavaScript, TypeScript, Stimulus controllers, Turbo Frames/Streams, or asset pipeline changes to evaluate.",
  "findings": []
}

Scope: Zero frontend surface area touched. No action required.

[kitcommerce]

♿ Accessibility Review

{
  "reviewer": "accessibility",
  "verdict": "PASS",
  "severity": null,
  "summary": "No user-facing changes in this PR. All modifications are backend/infrastructure only: CI matrix configuration, a Redis boot guard, a Sidekiq require guard, and a Rails 7.0 gemfile. Zero HTML, views, templates, ARIA attributes, CSS, or JavaScript was touched. No accessibility impact.",
  "findings": []
}

[kitcommerce]

✅ All Review Waves Passed — Merge Ready

All reviewers returned PASS or PASS_WITH_NOTES. This PR is merge-ready.

Wave	Reviewer	Verdict	Severity
Wave 1	architecture	PASS	—
Wave 1	simplicity	PASS	—
Wave 1	security	PASS	—
Wave 1	rails-conventions	PASS	—
Wave 2	rails-security	PASS	—
Wave 2	database	PASS	—
Wave 3	test-quality	PASS_WITH_NOTES	LOW
Wave 3	performance	PASS	—
Wave 3	frontend	PASS	—
Wave 3	accessibility	PASS	—
Wave 4	documentation	🔄 pending	—

Wave 4 (documentation) is informational only and does not block merge.

Notable LOW finding (test-quality): The audit result "zero ActiveRecord::Type usage" has no enforcement mechanism. Optional future work: add a CI grep step to lock it in. Not blocking.

Labeled merge:ready + merge:hold (60-min hold window). Awaiting hold window before auto-merge.

[kitcommerce]

{"reviewer":"documentation","verdict":"PASS_WITH_NOTES","severity":"LOW","summary":"PR description is clear and includes verification steps. No public API docs appear necessary for the small boot/CI changes; only minor note is that the new Rails 7.0 Gemfile and the boot-time guards may warrant a brief mention in contributor/docs if you maintain a list of supported Rails gemfiles.","findings":["PR description: Good structure (what/why/verification/client impact).","Public API docs: No new public methods/classes introduced; changes are internal boot/config guards.","Minor: Consider adding/confirming a short note somewhere contributor-facing (if such docs exist) that gemfiles/rails_7_0.gemfile is now part of the Rails matrix (experimental). Not required for merge."]}