Skip to content

Commit 4406ae3

Browse files
etagwerkerclaude
andcommitted
Merge origin/main into feature/rails-40-patterns-pr8; harden new patterns
Resolve the interleaved add/add conflict in rails-40-patterns.yml by taking main's file and re-inserting the three new high_priority entries intact (COLLECTION_PROXY_ENUMERABLE_GUARD, HWIA_CONVERT_VALUE_OVERRIDE, RACK_LOCK_MIDDLEWARE). Disjoint from main's accumulated patterns. Harden the three regexes from review (each was too literal for its idiom space): - COLLECTION_PROXY: also match `kind_of?` and the no-paren `is_a? Enumerable` form; \b avoids Enumerable-prefixed names. (Enumerable === / case-when stays out of scope, noted in the entry.) - HWIA: match a 1-arg override under any parameter name (not just `value`); the correct 2-arg form is still not flagged. - RACK_LOCK: also match the leading-`::` and string target forms (`::Rack::Lock`, `"Rack::Lock"`); still excludes index-insert / delete / use. - Add `exclude: "^\s*#"` to all three so commented-out lines don't flag. Add the required match/no_match fixtures for all three (including the widened idioms and the not-flagged forms as regression guards). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 parents 98d99ba + 61ec405 commit 4406ae3

12 files changed

Lines changed: 1057 additions & 32 deletions

File tree

.github/workflows/validate-patterns.yml

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,15 +4,21 @@ on:
44
push:
55
branches: [main]
66
paths:
7+
- "rails-upgrade/**/*.md"
78
- "rails-upgrade/detection-scripts/patterns/*.yml"
9+
- "upgrade-cleanup/**/*.md"
810
- "bin/validate-patterns"
911
- "bin/test-patterns"
12+
- "bin/lint-skill"
1013
- ".github/workflows/validate-patterns.yml"
1114
pull_request:
1215
paths:
16+
- "rails-upgrade/**/*.md"
1317
- "rails-upgrade/detection-scripts/patterns/*.yml"
18+
- "upgrade-cleanup/**/*.md"
1419
- "bin/validate-patterns"
1520
- "bin/test-patterns"
21+
- "bin/lint-skill"
1622
- ".github/workflows/validate-patterns.yml"
1723

1824
permissions:
@@ -30,3 +36,4 @@ jobs:
3036
- run: ruby bin/validate-patterns --self-test
3137
- run: ruby bin/test-patterns
3238
- run: ruby bin/test-patterns --self-test
39+
- run: ruby bin/lint-skill

CLAUDE.md

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,13 @@ This file captures project-specific conventions Claude should follow when workin
1111
- Checks: YAML parses, required top-level keys present, the eight required pattern keys present on each entry, every `pattern` / `exclude` regex compiles, and the `kind:` value is one of the allowed enum values (`breaking`, `deprecation`, `migration`, `optional`)
1212
- Exits 0 on success, 1 on any failure with a per-file error report
1313

14+
- `bin/test-patterns` runs fixture tests for a pattern file against its sibling `*.expectations.yml` (e.g. `rails-40-patterns.yml` + `rails-40-patterns.expectations.yml`). It confirms a pattern's regex actually matches what its explanation claims and skips what it shouldn't — `validate-patterns` only confirms the regex compiles, not that it's correct. Pure-stdlib Ruby, same shape as `bin/validate-patterns`.
15+
- `bin/test-patterns` tests every pattern file that has an expectations sibling; files without one are reported `SKIP`, not a failure
16+
- `bin/test-patterns path/to/file.yml` tests one or more specific files
17+
- `bin/test-patterns --self-test` runs built-in fixture assertions. CI runs this alongside the fixture-test step
18+
- Expectations are keyed by `variable_name` and list `match` (lines the pattern MUST flag) and `no_match` (lines it MUST NOT flag) — see the worked example in `rails-40-patterns.expectations.yml`
19+
- Exits 0 on success, 1 on any failure with a per-pattern error report
20+
1421
## Version guides (`rails-upgrade/version-guides/*.md`)
1522

1623
- **Do NOT include "Difficulty" or "Estimated Time" in the header.** These are subjective, application-dependent, and drift out of date. Keep the header minimal: title, Ruby requirement, and the attribution line.
@@ -26,6 +33,7 @@ This file captures project-specific conventions Claude should follow when workin
2633
- Each pattern needs: `name`, `kind` (one of `breaking` / `deprecation` / `migration` / `optional` — see "Assigning kind" below), `pattern` (regex), `exclude` (regex, empty string if none), `search_paths`, `explanation`, `fix`, `variable_name`. Place `kind:` immediately after `name:` for visual scannability.
2734
- Include a `dependencies` section for any bridge/compatibility gems mentioned in the guide.
2835
- **Required before committing any change to a detection pattern file:** run `bin/validate-patterns` (or `bin/validate-patterns path/to/file.yml` for the file you touched). Do not commit a pattern change without a clean run; broken YAML or schema drift in this directory breaks the skill at runtime. See the `## Repository tooling` section above for what the script checks.
36+
- **New pattern? Add a fixture expectation.** Every new `variable_name` needs a `match`/`no_match` entry in the sibling `*.expectations.yml` file (create the file if the version doesn't have one yet), then run `bin/test-patterns path/to/file.yml` clean before committing. Exception: a pattern whose `pattern` is `""` (pure path-based detection, e.g. `VENDOR_PLUGINS`) can never flag a line and should stay uncovered — note why in a comment instead of forcing a fixture.
2937

3038
## Assigning priority (🔴 HIGH / 🟡 MEDIUM / 🟢 LOW)
3139

@@ -71,6 +79,10 @@ Priority is about **urgency during an upgrade**, not editorial weight.
7179

7280
`kind:` describes **what the change is**; `priority` describes **how urgent it is**. The two are orthogonal. A HIGH `deprecation` (silently wrong, like `DIRTY_TRACKING_AFTER_SAVE`) and a HIGH `breaking` (won't boot) are both "fix first" but for different reasons.
7381

82+
**Judge `kind` at the target hop, not the API's historical timeline.** Each `rails-XY-patterns.yml` file is a statement *about that hop* — what changes when the user upgrades INTO that version. A removal that was first deprecated in an earlier Rails minor is `breaking` in the file for the version where it actually raises, not `deprecation` because of its history. The same API can legitimately be `deprecation` in `rails-31-patterns.yml` and `breaking` in `rails-40-patterns.yml`. Apply the rule to all four kinds: `kind` reflects what the change *is at this hop*, not what it *was* earlier or *will become* later.
83+
84+
Concrete example: `SCOPE_WITHOUT_LAMBDA` was deprecated in Rails 3.1 and raises in 4.0 — it is `breaking` in `rails-40-patterns.yml`.
85+
7486
The four values:
7587

7688
- **`breaking`** — Raises, removed, or prevents the app from booting / bundling / running its test suite. The user cannot complete the upgrade without addressing it. Example: `update_attributes` removed in 6.1, `redirect_to :back` removed in 5.1.
@@ -86,3 +98,30 @@ How to decide:
8698
4. **Is this purely opt-in / cosmetic / a new feature?**`optional`.
8799

88100
If `kind` and `priority` seem to conflict, trust both. They answer different questions.
101+
102+
## How the `dependencies:` section relates to `kind`
103+
104+
The top-level `dependencies:` block in each `rails-*-patterns.yml` file is not bound to a single `kind` value. It serves two distinct purposes:
105+
106+
1. **Bridge / compatibility gems for `breaking` patterns** — gems that rescue functionality removed from Rails core, so the user can keep shipping while migrating call sites. A `breaking` pattern with a corresponding bridge entry is a *softenable* break: install the gem to keep the upgrade landing while migration happens separately. Examples:
107+
- `protected_attributes` rescues `attr_accessible` / `attr_protected` (4.0)
108+
- `activerecord-deprecated_finders` rescues removed dynamic finders (4.0)
109+
- `rails-observers` rescues `ActiveRecord::Observer` and `ActionController::Caching::Sweeper` (4.0)
110+
- `responders` rescues `respond_with` and class-level `respond_to` (4.2)
111+
- `rails-controller-testing` rescues `assigns` / `assert_template` (5.0)
112+
113+
2. **New gems Rails introduces or recommends at this version** — gems that are not in the previous version's Gemfile. These pair with `optional` patterns (the user can ignore them) or with no pattern at all. Examples:
114+
- `bootsnap` (5.2), `web-console` (4.2), `webpacker` (5.1)
115+
- `propshaft` (8.0), `solid_cache` / `solid_queue` / `solid_cable` (8.0)
116+
- `kamal` (8.0), `bundler-audit` (8.1)
117+
118+
The `check: true` / `check: false` flag on each `dependencies:` entry is **editorial advice about whether the gem applies broadly**, not a per-app determination. The actual applicability depends on whether the user's app triggers the `breaking` pattern the bridge rescues:
119+
120+
- `check: true` — the gem rescues a `breaking` that most apps will trigger (e.g., `responders` rescues `respond_with`, which most controller-heavy apps use). Default to recommending it.
121+
- `check: false` — either a bridge for a `breaking` that not all apps will trigger (e.g., `actionpack-action_caching` only matters if the app uses `caches_page` / `caches_action`), or a new-default gem the user can adopt at their own pace (e.g., `bootsnap`, `solid_cache`). The user's actual code drives whether they need it.
122+
123+
A `check: false` bridge gem still becomes effectively required for any specific app that triggers its rescued `breaking` pattern. Treat the flag as a starting recommendation; the per-app `fix:` field on each pattern entry is what tells the user whether they actually hit it.
124+
125+
`kind: deprecation`, `migration`, and `optional` patterns are resolved in code via the per-pattern `fix:` field, not via `dependencies:`.
126+
127+
`bin/validate-patterns` does not enforce the rules in this section — they are editorial guidance for authors and reviewers. The validator only checks schema (required keys, regex compilation, `kind:` enum membership), not the semantic relationship between `dependencies:` entries and `kind:` values.

README.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,7 @@ We welcome contributions! Here's how you can help:
172172
- Keep content factual and based on official Rails documentation
173173
- Include code examples with BEFORE/AFTER patterns
174174
- Test detection patterns against real codebases when possible
175+
- New detection pattern? Add a `match`/`no_match` fixture to its `*.expectations.yml` file and run `bin/test-patterns` before opening a PR
175176
- Attribute sources appropriately
176177

177178
## License

bin/lint-skill

Lines changed: 198 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,198 @@
1+
#!/usr/bin/env ruby
2+
# frozen_string_literal: true
3+
4+
# Consistency linter for the rails-upgrade and upgrade-cleanup skills.
5+
#
6+
# Catches drift classes that are easy to introduce by hand and easy to miss
7+
# in review: broken file-path references in prose, orphaned reference /
8+
# workflow / version-guide files, SKILL.md index rows that don't match what's
9+
# on disk, gaps in "Step N" header numbering, and pattern-catalog filenames
10+
# whose derived version disagrees with their own `version:` key.
11+
#
12+
# Usage: ruby bin/lint-skill (exits 1 on any error)
13+
14+
require "yaml"
15+
16+
ROOT = File.expand_path("..", __dir__)
17+
18+
SKILLS = {
19+
"rails-upgrade" => File.join(ROOT, "rails-upgrade"),
20+
"upgrade-cleanup" => File.join(ROOT, "upgrade-cleanup", "upgrade-cleanup")
21+
}.freeze
22+
23+
$errors = []
24+
25+
def err(file, detail)
26+
$errors << "#{file.sub("#{ROOT}/", "")}: #{detail}"
27+
end
28+
29+
def markdown_files_for(skill_dir)
30+
Dir.glob(File.join(skill_dir, "**", "*.md")).sort
31+
end
32+
33+
# ---------------------------------------------------------------------------
34+
# 1. File-path tokens in prose resolve to real files on disk
35+
# ---------------------------------------------------------------------------
36+
PATH_TOKEN = %r{(?:references|workflows|version-guides|templates|examples|detection-scripts/patterns)/[A-Za-z0-9_./-]+\.(?:md|yml)}
37+
38+
SKILLS.each_value do |skill_dir|
39+
markdown_files_for(skill_dir).each do |f|
40+
next if File.basename(f) == "CHANGELOG.md" # historical record — old paths intentionally no longer exist
41+
42+
File.read(f).each_line.with_index(1) do |line, ln|
43+
next if line.include?(".github/") # e.g. `.github/workflows/ci.yml` — CI config, not a skill path
44+
next if line.include?("dual-boot") # cross-plugin reference into the external dual-boot skill repo
45+
46+
line.scan(PATH_TOKEN) do |token|
47+
next if token.include?("{") || token.include?("<") || token.include?("*")
48+
49+
path = File.join(skill_dir, token)
50+
err(f, "line #{ln}: path `#{token}` does not exist") unless File.exist?(path)
51+
end
52+
end
53+
end
54+
end
55+
56+
# ---------------------------------------------------------------------------
57+
# 2. No orphaned reference / workflow / version-guide / template / example files
58+
# ---------------------------------------------------------------------------
59+
ORPHAN_CHECK_DIRS = %w[references workflows version-guides templates examples].freeze
60+
61+
SKILLS.each do |skill_name, skill_dir|
62+
corpus = markdown_files_for(skill_dir).map { |f| [f, File.read(f)] }
63+
64+
ORPHAN_CHECK_DIRS.each do |dir|
65+
Dir.glob(File.join(skill_dir, dir, "*.md")).sort.each do |file|
66+
base = File.basename(file)
67+
mentioned = corpus.any? { |f, content| f != file && content.include?(base) }
68+
err(file, "orphaned — no inbound pointer from any file in the #{skill_name} skill") unless mentioned
69+
end
70+
end
71+
end
72+
73+
# ---------------------------------------------------------------------------
74+
# 3. SKILL.md bullet index matches files on disk (workflows/references/examples)
75+
# ---------------------------------------------------------------------------
76+
INDEX_CHECK_DIRS = %w[workflows references examples].freeze
77+
78+
SKILLS.each do |skill_name, skill_dir|
79+
skill_md = File.join(skill_dir, "SKILL.md")
80+
next unless File.exist?(skill_md)
81+
82+
content = File.read(skill_md).each_line.reject { |l| l.include?("dual-boot") }.join
83+
84+
INDEX_CHECK_DIRS.each do |dir|
85+
on_disk = Dir.glob(File.join(skill_dir, dir, "*.md")).map { |f| File.basename(f) }.sort
86+
next if on_disk.empty?
87+
88+
listed = content.scan(%r{`#{dir}/([A-Za-z0-9_./-]+\.md)`}).flatten.sort.uniq
89+
90+
(on_disk - listed).each do |missing|
91+
err(skill_md, "#{dir}/#{missing} exists on disk but is not listed in SKILL.md")
92+
end
93+
(listed - on_disk).each do |stale|
94+
err(skill_md, "SKILL.md lists `#{dir}/#{stale}` but the file does not exist on disk")
95+
end
96+
end
97+
end
98+
99+
# ---------------------------------------------------------------------------
100+
# 4. "Step N" headers run 1..N per workflow file, no gaps or duplicates
101+
# ---------------------------------------------------------------------------
102+
SKILLS.each_value do |skill_dir|
103+
Dir.glob(File.join(skill_dir, "workflows", "*.md")).sort.each do |f|
104+
steps = File.read(f).scan(/^(?:##|###)\s+Step (\d+)[:\s]/).flatten.map(&:to_i)
105+
next if steps.empty?
106+
107+
expected = (1..steps.length).to_a
108+
err(f, "step headers are #{steps.inspect}, expected #{expected.inspect}") unless steps == expected
109+
end
110+
end
111+
112+
# ---------------------------------------------------------------------------
113+
# 5. SKILL.md frontmatter `name:` matches its parent directory name
114+
# ---------------------------------------------------------------------------
115+
SKILLS.each_value do |skill_dir|
116+
skill_md = File.join(skill_dir, "SKILL.md")
117+
next unless File.exist?(skill_md)
118+
119+
content = File.read(skill_md)
120+
frontmatter = content[/\A---\n(.*?)\n---/m, 1]
121+
122+
if frontmatter.nil?
123+
err(skill_md, "missing YAML frontmatter (--- name: ... ---)")
124+
next
125+
end
126+
127+
name = YAML.safe_load(frontmatter)["name"]
128+
expected = File.basename(skill_dir)
129+
130+
err(skill_md, "frontmatter name `#{name}` does not match parent directory `#{expected}`") if name != expected
131+
end
132+
133+
# ---------------------------------------------------------------------------
134+
# 6. Version-guide hop chain is unbroken (each hop's "to" == next hop's "from")
135+
# ---------------------------------------------------------------------------
136+
VERSION_GUIDES_DIR = File.join(SKILLS.fetch("rails-upgrade"), "version-guides")
137+
version_key = ->(v) { v.split(".").map(&:to_i) }
138+
139+
hops = Dir.glob(File.join(VERSION_GUIDES_DIR, "upgrade-*-to-*.md")).sort.filter_map do |f|
140+
m = File.basename(f).match(/\Aupgrade-(\d+\.\d+)-to-(\d+\.\d+)\.md\z/)
141+
if m.nil?
142+
err(f, "filename doesn't match upgrade-<FROM>-to-<TO>.md")
143+
next
144+
end
145+
{ file: f, from: m[1], to: m[2] }
146+
end
147+
148+
hops.sort_by! { |h| version_key.call(h[:from]) }
149+
150+
hops.each_cons(2) do |prev, cur|
151+
next if prev[:to] == cur[:from]
152+
153+
err(cur[:file], "hop starts at #{cur[:from]} but the prior hop (#{File.basename(prev[:file])}) ends at #{prev[:to]} — gap or overlap in the version-guide chain")
154+
end
155+
156+
# ---------------------------------------------------------------------------
157+
# 7. Pattern catalog filename <-> version sanity
158+
# ---------------------------------------------------------------------------
159+
PATTERNS_DIR = File.join(SKILLS.fetch("rails-upgrade"), "detection-scripts", "patterns")
160+
161+
Dir.glob(File.join(PATTERNS_DIR, "rails-*-patterns.yml")).sort.each do |f|
162+
digits = File.basename(f)[/rails-(\d+)-patterns\.yml/, 1]
163+
164+
if digits.nil?
165+
err(f, "filename doesn't match rails-<MAJORMINOR>-patterns.yml")
166+
next
167+
end
168+
169+
if digits.length != 2
170+
err(f, "cannot derive a dotted version from #{digits.length} digits (#{digits}) — update the MAJORMINOR mapping in bin/lint-skill")
171+
next
172+
end
173+
174+
derived_version = "#{digits[0]}.#{digits[1]}"
175+
176+
begin
177+
doc = YAML.safe_load_file(f)
178+
rescue Psych::SyntaxError => e
179+
err(f, "YAML parse error: #{e.message}")
180+
next
181+
end
182+
183+
next unless doc.is_a?(Hash)
184+
185+
declared_version = doc["version"]
186+
if declared_version && declared_version.to_s != derived_version
187+
err(f, "filename implies version #{derived_version} but `version:` key says #{declared_version.inspect}")
188+
end
189+
end
190+
191+
# ---------------------------------------------------------------------------
192+
if $errors.empty?
193+
puts "lint-skill: OK"
194+
else
195+
$errors.each { |e| puts "ERROR #{e}" }
196+
puts "lint-skill: #{$errors.length} error(s)"
197+
exit 1
198+
end

rails-upgrade/CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
- Threaded the `kind` / bucket taxonomy through `rails-upgrade/SKILL.md` so the executive-summary file uses the same vocabulary as `workflows/direct-detection-workflow.md` (issue #53 rollout, sub-issue #94, follow-up to #69). Step 5's report description now names both buckets, Step 6 describes implementation as "fix-before-bump fixes (`kind: breaking` and `kind: deprecation`)", the Common Request Patterns block was updated to match, and the Quality Checklist now references the two-bucket grouping. The Step 6 caveat about "deprecations printed by the next version" was reworded to make explicit that it covers `load_defaults`-triggered runtime warnings about *future* Rails versions (Step 7's domain) — not the `kind: deprecation` detection patterns from Step 4, which remain part of fix-before-bump.
88
- Updated `workflows/direct-detection-workflow.md` to surface the new `kind:` field in detection output (issue #53 rollout, sub-issue #69). Findings are now grouped into two top-level buckets: **fix-before-bump** (`kind: breaking` and `deprecation`) and **fix-when-ready** (`kind: migration` and `optional`). Within each bucket, `priority` (HIGH / MEDIUM / LOW) drives sub-ordering. Putting `breaking` and `deprecation` together reflects the practical reality that deprecations warn in production logs at this hop and typically become hard breaks at the next — addressing them in the same upgrade campaign is cheaper than splitting the work across two upgrades. `migration` and `optional` are silent at this hop and don't compete for the user's attention during the upgrade itself.
99
- Added a `--self-test` flag to `bin/validate-patterns` (closes #71). Running `bin/validate-patterns --self-test` writes valid + invalid fixture YAMLs to a tmpdir and asserts the validator's behavior end-to-end: four positive cases (one per `kind:` value) and four rejection paths (missing top-level key, missing required pattern key, broken regex, unknown `kind:` value). Wired into the GitHub Actions workflow. Closes the test-coverage gap surfaced during PR #70 review where the new enum guard had no automated coverage. Single-file, stdlib-only — no Bundler, no test framework.
10+
- Documented two implicit rules in `CLAUDE.md`'s `## Assigning kind:` section (closes #76): (1) **judge `kind` at the target hop** — each `rails-XY-patterns.yml` is a statement about that specific hop, so the same API can be `deprecation` in `rails-31-patterns.yml` and `breaking` in `rails-40-patterns.yml`; (2) **`dependencies:``kind` relationship** — the top-level `dependencies:` block covers both bridge gems for `breaking` patterns (e.g., `protected_attributes` for `attr_accessible`) and new-default / new-recommended gems Rails introduces at this version (e.g., `bootsnap`, `propshaft`, the `solid_*` family). Closes the gaps surfaced during reviews of #73 (target-hop) and #74 (`dependencies:` semantics).
1011

1112
## v3.3.0, 28 April 2026
1213
- Added `upgrade-cleanup` companion plugin for finishing a Rails upgrade campaign. Removes dual-boot scaffolding, drops `NextRails.next?` / `NextRails.current?` branches, retires stale monkey-patches and version-conditional code, and aligns CI matrix / Dockerfile / Ruby pin to the new version baseline. Lives as a sibling plugin in this repo (`upgrade-cleanup/.claude-plugin/plugin.json` + `upgrade-cleanup/upgrade-cleanup/SKILL.md` + `upgrade-cleanup/upgrade-cleanup/workflows/upgrade-cleanup-workflow.md`). `load_defaults` alignment and deprecation triage stay with the rails-upgrade skill, cleanup deliberately does not duplicate them. Based on FastRuby.io's "Finishing an Upgrade" methodology. (Closes #1)

0 commit comments

Comments
 (0)