Harden scope/default_scope detection across the 4.x hops - #128
Closed
JuanVqz wants to merge 1 commit into
Closed
Conversation
A non-callable scope body is not a style issue on Rails 4.0. 3.2 wrapped even an eager body in a lambda and merged it into `scoped` at call time, so an association's condition survived. 4.0 returns the stored relation verbatim on the non-callable branch (no `merge`, no `scoping`), so everything to the left of the scope is discarded: `incident.issues.active` drops `incident_id` and reads the whole table, and `Model.where(x).active` drops `where(x)`. Rails only warns, at class-definition time. This shipped to production in a real 3.2 -> 4.0 upgrade. A page called `incident.issues.active` to decide whether a record was editable; the unbounded read killed the worker and surfaced as a 503. Dev and CI had too little data for the query to be slow, so every pre-production gate passed. The guide presented this as a mechanical rewrite and the detection regex only looked for `where`. Both understated it. - SCOPE_WITHOUT_LAMBDA keeps kind: breaking at 4.0, with an inline comment against reclassifying it to deprecation on the strength of the warning. CLAUDE.md gains the general rule: a deprecation that changes query results is breaking at the hop where the behavior changes. - Widened SCOPE_WITHOUT_LAMBDA from `scope :x, where` to any non-callable body, and DEFAULT_SCOPE_WITHOUT_BLOCK from `where|order|:order` to any non-block argument. Both excludes are anchored to the body position and tolerate a wrapping paren. Measured on a ~900-scope Packwerk codebase: 17 scope findings before, 23 after (it was missing `order(...)`, `joins(...)`, `scoped.where(...)`), and 0 false positives on the same codebase once swept. The `\b` guard on default_scope came from that run too — `default_scoper` was matching. - Added SCOPE_BODY_CONTINUATION for a body that wraps onto the next line, which a line-oriented pattern cannot classify. On the audited codebase 22 of 25 non-callable scopes were single-line; all 3 misses were this shape and two were live bugs. - Added SCOPE_NAME_COLLISION for the name guard 4.1 added to #scope. `scope :none` is the usual casualty: Active Record gained Model.none in 4.0, so a hand-rolled 3.2-era scope carries a name that is now taken. - Each hop now states its own symptom, in both the pattern file and the guide: 4.0 warns and is silently wrong (section 3a), 4.1 raises NoMethodError at call time (7b) and rejects colliding names (7c), 4.2 raises ArgumentError at class load (5b, 5c). - Section 3a carries the 3.2 vs 4.0 `#scope` source, the before/after to_sql, a per-version behavior table, and an auditing order that starts with association call sites — the only places the bug is observable. Version behavior verified against the vendored activerecord 3.2.22.5 and 4.0.13 gems and the tagged v4.1.0 / v4.2.0 sources; the SQL was measured by running both boots, not inferred. Corrects the CLAUDE.md example that claimed this was deprecated in 3.1 and raised in 4.0 — 3.1 had no deprecation, and 4.0 warns.
JuanVqz
marked this pull request as ready for review
August 5, 2026 18:01
Member
Author
|
This seems app-related, not general, so closing it |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
A non-callable scope body is not a style issue on Rails 4.0. It silently returns wrong rows, and the skill treated it as a mechanical rewrite.
Rails 3.2 merged the stored relation into the current scope at call time, so an association's condition survived. Rails 4.0 returns it verbatim on the non-callable branch — no
merge, noscoping— so everything to the left is discarded:Rails only warns, at class-definition time. This shipped to production in a real 3.2 → 4.0 upgrade: a page called
incident.issues.activeto decide whether a record was editable, the unbounded read killed the worker, and it surfaced as a 503. Dev and CI held too little data for the query to be slow, so every pre-production gate passed.The old detection regex looked only for
where, so it also missedorder(...),joins(...)andscoped.where(...).What changed
Widened detection.
SCOPE_WITHOUT_LAMBDAnow matches any non-callable body, andDEFAULT_SCOPE_WITHOUT_BLOCKany non-block argument (it waswhere|order|:order, missingdefault_scope joins(...)and friends). Both excludes are anchored to the body position and tolerate a wrapping paren.New patterns.
SCOPE_BODY_CONTINUATIONfor a body that wraps onto the next line, which a line-oriented pattern cannot classify — flagged for human review rather than dropped.SCOPE_NAME_COLLISIONfor the name guard 4.1 added to#scope;scope :noneis the usual casualty, since Active Record gainedModel.nonein 4.0 and a hand-rolled 3.2-era scope now collides.Per-hop coverage. The same defect has a different symptom at each version, so each hop's pattern file and guide carries its own entry:
NoMethodError, lazily on first call; colliding names rejectedArgumentErrorat class loadGuides. Section 3a now carries the 3.2-vs-4.0
#scopesource, the measuredto_sql, a per-version behavior table, and an auditing order that starts with association call sites — the only places where the bug is observable. Plus troubleshooting rows for the symptoms as an operator actually meets them: a query returning the whole table, an unexplained 503,ArgumentError: The scope body needs to be callable.CLAUDE.md. Newkind:rule: a deprecation that changes query results isbreakingat the hop where the behavior changes. The deciding question is whether a green suite and a clean boot can hide it, not whether Rails prints a warning. Without this, the rubric's rule 2 argues for downgrading the 4.0 entry todeprecation. Also corrected the section's example, which claimed this API was deprecated in 3.1 and raised in 4.0 — verified against the gems: 3.1 had no deprecation, and 4.0 warns.Verification
Behavior confirmed against the vendored activerecord 3.2.22.5 and 4.0.13 gems and the tagged v4.1.0 / v4.2.0 sources. The SQL was measured by running both boots of a real dual-boot app, not inferred.
Both regexes were run against a ~900-scope Packwerk codebase before and after its own sweep:
default_scope: 0 of 7 declarations flagged (all block form), after a\bguard was added — the first run matched adefault_scoperhelper callBoth false positives that run surfaced (a parenthesised
procbody,default_scoper) are now fixtures.bin/validate-patterns,bin/test-patterns(both with--self-test) andbin/lint-skillpass.Note for reviewers
SCOPE_WITHOUT_LAMBDAstayskind: breakingat the 4.0 hop even though Rails 4.0 only warns. That is deliberate and the pattern entries carry inline comments saying so — the failure mode is wrong query results, which a passing test suite hides.DEFAULT_SCOPE_WITHOUT_BLOCKis also keptbreakingat 4.0, where it is genuinely deprecated-but-correct, so that it gets fixed in the same pass as the scope bodies rather than sending someone back over the same models at 4.1.