Add detection patterns for Rails 5.1 → 5.2 upgrade

[etagwerker]

Summary

Closes #15. Adds rails-upgrade/detection-scripts/patterns/rails-52-patterns.yml. 14 patterns + 1 dependency. YAML parses, all regexes compile under Ruby Onigmo.

Sources

• Official Rails 5.2 upgrade guide (§ Upgrading from Rails 5.1 to Rails 5.2)
• FastRuby.io 5.1 → 5.2 upgrade blog post
• FastRuby internal upgrade-template tickets (5319, 5321, 5322, 5323, 5803, 5805, 5806, 5807, 5808, 5809, 3358, 3395) — concrete deprecations the team tracks on every 5.1 → 5.2 engagement

The official upgrade guide section is unusually thin (only 2 subsections: Bootsnap and cookie expiry format). FastRuby's blog confirms it: "There were not too many deprecations between 5.1 and 5.2 compared to past releases." The bulk of actionable detection comes from the FastRuby internal tickets.

Patterns

🔴 HIGH (1)

• DIRTY_TRACKING_AFTER_SAVE (FastRuby ticket 3358) — *_changed?, *_was, .changes, .changed? return false / nil after save. Whether each match is broken depends on whether the call runs before or after commit. NOISY REGEX with audit-each-match guidance in the explanation. HIGH because the failure mode is silently wrong production behavior in after_save / after_commit callbacks, observers, or jobs that read attribute state post-persistence.

🟡 MEDIUM (6)

• DYNAMIC_ROUTE_SEGMENTS (5321) — :controller / :action route segments
• CLASS_NAME_CONSTANT (3395) — unquoted constants in class_name:
• ERROR_ON_IGNORED_ORDER_OR_LIMIT (5323) — config rename
• HALT_CALLBACK_CHAINS_ON_RETURN_FALSE (5807) — dead config (no-op since 5.0)
• CALLBACK_STRING_CONDITIONS (5808) — model-callback variant of the string-if: deprecation. The controller-filter variant lives in PR Add Rails 5.0 → 5.1 detection patterns #38's 5.1 patterns file; this entry covers the model-callback case so it's caught before final removal.
• SECRETS_USAGE — Rails.application.secrets.* references; migration target for credentials.yml.enc

🟢 LOW (7)

• BOOTSNAP_PRESENCE — Gemfile presence check; absence is the signal
• EVENT_REDIS_ADAPTER (5319) — deprecated ActionCable adapter
• ERUBIS_HANDLER (5322) — explicit Erubis references
• QUOTED_ID_METHOD (5805) — legacy AR override
• VERIFY_METHOD_DEPRECATION (5803) — NOISY REGEX (most .verify( calls are unrelated)
• LOCK_BANG_UNPERSISTED (5806) — NOISY REGEX (audit each call)
• LEGACY_SECRET_KEY_BASE (5809) — best-effort: matches legacy Rails.application.config.secret_token / secret_key_base reads. Absence does not prove correctness.

Dependencies

• bootsnap (check: false) — new 5.2 default; opt-in performance gem.

Priority calibration

• Dirty tracking → 🔴 HIGH, not MEDIUM. Per the CLAUDE.md rubric, "silently wrong behavior with production impact" is HIGH. Whether a *_changed? call is broken depends on the before-vs-after-commit context, which is not visible from the regex match itself.
• Bootsnap → 🟢 LOW, not 🔴 HIGH as the in-repo guide suggests. Bootsnap is opt-in per the official upgrade guide ("If you want to use it…"). Demoting per the rubric's "opt-in or optional improvement" rule.
• Cookie expiry format change — no actionable code detection (handled by use_authenticated_cookie_encryption flag for staged rollback). Skipped.

Discrepancies with version-guides/upgrade-5.1-to-5.2.md

While drafting, I noticed several items in the in-repo guide that look misattributed against primary sources. Listing them here so a future PR can revise the guide; out of scope for this patterns-only PR:

• attribute_changed? after save — the behavior change actually landed in Rails 5.1, not 5.2 (it's in PR Add Rails 5.0 → 5.1 detection patterns #38's scope). At 5.2 it remains a deprecation warning. Detection still belongs here as a final reminder before later-version removal, which is what DIRTY_TRACKING_AFTER_SAVE does.
• force_ssl 301 redirect — not in either primary source for 5.1 → 5.2; predates 5.2.
• per_form_csrf_tokens default flip — only triggers via config.load_defaults 5.2, which is Step 6 of the workflow (post-upgrade), not breaking-change detection in Step 5.
• Active Storage / CSP DSL marked as breaking — both are new features, not breaking changes.

Test plan

• ruby -e "require 'yaml'; YAML.safe_load_file('rails-upgrade/detection-scripts/patterns/rails-52-patterns.yml')" succeeds
• All 14 regexes compile under Ruby Onigmo
• Manual smoke: run detection on a Rails 5.1 app and confirm DIRTY_TRACKING_AFTER_SAVE, DYNAMIC_ROUTE_SEGMENTS, CALLBACK_STRING_CONDITIONS, and SECRETS_USAGE fire on representative call sites
• Cross-check against RailsDiff 5.1.7 → 5.2.8 for anything missed

[Copilot]

Pull request overview

Adds a new detection-patterns YAML file to support identifying Rails 5.1 → 5.2 upgrade risks via regex-based scanning.

Changes:

• Introduces rails-52-patterns.yml with HIGH/MEDIUM/LOW breaking-change detection patterns for Rails 5.2.
• Adds a bootsnap dependency entry (opt-in) plus a corresponding Gemfile pattern entry.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

DYNAMIC_ROUTE_SEGMENTS will also match old-style hash-rocket route options like :controller => 'users' / :action => 'index', which are not the deprecated dynamic segment shape. The current exclude only covers controller:/action: keyword syntax, so this is likely to create false positives. Consider extending exclude to filter :controller\s*=> and :action\s*=> as well.

Suggested change

	exclude: "controller:|action:|action_dispatch|action_view|action_mailer|action_cable|action_pack|action_controller"
	exclude: "controller:|action:|:controller\\s*=>|:action\\s*=>|action_dispatch|action_view|action_mailer|action_cable|action_pack|action_controller"

[Copilot]

The VERIFY_METHOD_DEPRECATION entry is specifically about passing arguments to ConnectionPool#verify, but the pattern \.verify\( matches no-arg calls too. That makes the suggested fix (“Drop the argument”) incorrect for some hits and increases noise. Consider tightening the regex to require at least one non-space character before the closing ) (or adjust the text to say “if arguments are passed”).

Suggested change

	pattern: "\\.verify\\("
	exclude: ""
	search_paths:
	- "app/"
	- "lib/"
	explanation: "Rails 5.2 deprecates passing arguments to ActiveRecord::ConnectionAdapters::ConnectionPool#verify (it ignored them anyway). NOISY REGEX: matches every `.verify(` call site, most of which are unrelated (signed cookies, OpenSSL keys, Devise, JWT, RSpec doubles). Filter results manually for AR connection-pool receivers"
	fix: "Drop the argument: connection_pool.verify(timeout) → connection_pool.verify"
	pattern: "\\.verify\\(\\s*\\S[^)]*\\)"
	exclude: ""
	search_paths:
	- "app/"
	- "lib/"
	explanation: "Rails 5.2 deprecates passing arguments to ActiveRecord::ConnectionAdapters::ConnectionPool#verify (it ignored them anyway). NOISY REGEX: matches `.verify(...)` call sites with at least one argument expression, many of which are still unrelated (signed cookies, OpenSSL keys, Devise, JWT, RSpec doubles). Filter results manually for AR connection-pool receivers"
	fix: "If arguments are passed, drop them: connection_pool.verify(timeout) → connection_pool.verify"

[Copilot]

LEGACY_SECRET_KEY_BASE includes secret_token\.rb in the content regex, but this won’t detect the presence of config/initializers/secret_token.rb (grep searches file contents, not filenames). If you want a file-presence/manual-review marker, consider scoping search_paths to config/initializers/secret_token.rb and using a generic marker pattern (or drop secret_token\.rb from the regex).

[Copilot]

DIRTY_TRACKING_AFTER_SAVE includes *_will_change! in the regex, but the explanation/fix are about post-save dirty tracking reads (*_changed?, *_was, .changes, .changed?) returning false/nil after persistence. *_will_change! is a pre-save marker method, so including it here likely creates unrelated hits without a corresponding fix. Consider removing will_change! from this pattern (or updating the explanation/fix to explicitly justify how it relates).

Suggested change

	pattern: "\\b\\w+_(changed\\?|was|will_change!)|\\.changes\\b|\\.changed\\?"
	pattern: "\\b\\w+_(changed\\?|was)|\\.changes\\b|\\.changed\\?"

[Copilot]

This patterns file doesn’t cover several items currently listed under “Breaking Changes” in rails-upgrade/version-guides/upgrade-5.1-to-5.2.md (e.g., Cookie expiry format change at ~lines 43-58; CSP DSL at ~134-150; force_ssl redirect status at ~154-166; per_form_csrf_tokens default at ~170-187; ActiveStorage notes at ~89-103). If the intent is to close issue #15’s “cover all breaking changes from the version guide” criterion, consider adding detection entries (even as FILE MARKER/manual-review patterns) or updating the guide so the two stay in sync.

[Copilot]

The LOCK_BANG_UNPERSISTED regex \.lock!(\b|\() won’t match the common record.lock! form without parentheses because \b can’t occur immediately after ! (non-word). This will miss many real call sites. Consider switching to a lookahead/end/whitespace check (e.g., allow (, whitespace, or end-of-line) so both lock! and lock!() are detected.

Suggested change

	pattern: "\\.lock!(\\b|\\()"
	pattern: "\\.lock!(?=\\(|\\s|$)"

[JuanVqz]

@etagwerker was about to code review this one but it seems like copilot has already some feedback.