From b297c7b0bd9151d491a2fed042dbf80e3fda3596 Mon Sep 17 00:00:00 2001 From: Mattia Giuffrida Date: Mon, 28 Sep 2026 18:07:43 +0100 Subject: [PATCH 1/4] Skip ERB comments when converting ERB to Ruby convert_erb_to_ruby_without_sourcemaps joins the contents of every ERB tag into one Ruby source. The first line of a `<%#` comment starts with `#`, so it became a Ruby comment, but every later line became Ruby code. What that did depended on the text: a trailing period turned the next tag into a method call, an apostrophe opened a string that swallowed the rest of the file, and `def x` made the whole file fail to parse. So references were missed with no error, and a constant named on a later comment line was reported as a reference. Comment tags are now dropped before the join, as packwerk's ERB parser does. Only a tag that opens with exactly `<%#` is a comment. `<%-#` and `<% #` are code tags in Erubi and in packwerk, and their later lines are Ruby, which is why the check is on the whole match and not the capture. Both ERB parsers go through this function, so both are fixed. Single-line comments are dropped too, which changes one case. A leading encoding comment such as `<%# encoding: iso-8859-1 %>` used to become the first line of the Ruby source, where lib-ruby-parser read it as a magic comment, and for any encoding but UTF-8 or ASCII-8BIT pks reported nothing for the file. Rails strips that tag before compiling (ActionView's ENCODING_TAG) and packwerk ignores it, so those files are now checked. Every other single-line comment was already a Ruby comment, so dropping it changes nothing. ERB references are all reported at 0:0, so the shorter Ruby source doesn't move any location. The first three new tests fail on main. The fourth passes on main and fails if the check moves to the capture. Cached results aren't tied to the pks version, so an unchanged ERB file keeps its old result after an upgrade. The CHANGELOG entry tells users to run `pks delete-cache`. Fixes #42. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 16 ++++ src/packs/file_utils.rs | 1 + src/packs/parsing/erb/packwerk.rs | 124 ++++++++++++++++++++++++++++++ 3 files changed, 141 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 231523cc..2beda7a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,22 @@ ### Fixes +#### ERB comments no longer hide or invent references + +pks read every line of a multi-line `<%# %>` comment after the first as Ruby +code. Depending on what the comment said, references elsewhere in the file were +missed, sometimes all of them, and constants named inside the comment were +reported as references. ERB comments are now skipped, as packwerk does. + +pks also found nothing at all in an ERB file whose first tag was an encoding +comment, such as `<%# encoding: iso-8859-1 %>`, unless it named UTF-8 or +ASCII-8BIT. Those files are now checked like any other, so they may show new +violations. + +Cached results aren't tied to the pks version, so an ERB file you haven't +changed keeps its old result after you upgrade. Unless you've turned the cache +off, run `pks delete-cache` once after upgrading so ERB files are read again. + #### `check -o json` and `-o csv` list each strict violation once A strict violation that fails the run is both a new violation and a strict-mode diff --git a/src/packs/file_utils.rs b/src/packs/file_utils.rs index 8b1a5003..ba497912 100644 --- a/src/packs/file_utils.rs +++ b/src/packs/file_utils.rs @@ -109,6 +109,7 @@ pub(crate) fn convert_erb_to_ruby_without_sourcemaps( let extracted_contents: Vec<&str> = regex .captures_iter(&contents) + .filter(|capture| !capture[0].starts_with("<%#")) .map(|capture| capture.get(1).unwrap().as_str()) .collect(); diff --git a/src/packs/parsing/erb/packwerk.rs b/src/packs/parsing/erb/packwerk.rs index 2c790837..12a160f7 100644 --- a/src/packs/parsing/erb/packwerk.rs +++ b/src/packs/parsing/erb/packwerk.rs @@ -234,4 +234,128 @@ mod tests { .unresolved_references ); } + + #[test] + fn multiline_comment_does_not_hide_surrounding_references() { + let configuration = Configuration::default(); + let expected: Vec = ["Foo", "Bar", "Baz"] + .iter() + .map(|name| UnresolvedReference { + name: String::from(*name), + namespace_path: vec![], + location: Range::default(), + }) + .collect(); + + for text in [ + "second line.", + "it's here", + "the end", + "see } here", + "second (line", + "class foo", + "def x", + ] { + let contents = format!( + "/ +<%= Foo %> +<%# first line + {text} %> +<%= Bar %> +<%= Baz %> + " + ); + assert_eq!( + expected, + process_from_contents( + contents, + &PathBuf::from("path/to/file.rb"), + &configuration + ) + .unresolved_references, + "comment ending with {text:?}" + ); + } + } + + #[test] + fn multiline_comment_contents_are_not_references() { + let contents: String = String::from( + "/ +<%# + Foo +%> +<%# uses + Baz -%> +<%= Bar %> + ", + ); + let configuration = Configuration::default(); + assert_eq!( + vec![UnresolvedReference { + name: String::from("Bar"), + namespace_path: vec![], + location: Range::default() + }], + process_from_contents( + contents, + &PathBuf::from("path/to/file.rb"), + &configuration + ) + .unresolved_references + ); + } + + #[test] + fn leading_encoding_comment_does_not_hide_references() { + let contents: String = + String::from("<%# encoding: iso-8859-1 %>\n<%= Foo %>"); + let configuration = Configuration::default(); + assert_eq!( + vec![UnresolvedReference { + name: String::from("Foo"), + namespace_path: vec![], + location: Range::default() + }], + process_from_contents( + contents, + &PathBuf::from("path/to/file.rb"), + &configuration + ) + .unresolved_references + ); + } + + #[test] + fn hash_after_hyphen_or_space_is_code_not_comment() { + let contents: String = String::from( + "/ +<%-# first line + Foo %> +<% # first line + Bar %> + ", + ); + let configuration = Configuration::default(); + assert_eq!( + vec![ + UnresolvedReference { + name: String::from("Foo"), + namespace_path: vec![], + location: Range::default() + }, + UnresolvedReference { + name: String::from("Bar"), + namespace_path: vec![], + location: Range::default() + } + ], + process_from_contents( + contents, + &PathBuf::from("path/to/file.rb"), + &configuration + ) + .unresolved_references + ); + } } From 7b3325450e74514f4c85f9eb69c98bfb7bc28831 Mon Sep 17 00:00:00 2001 From: Mattia Giuffrida Date: Tue, 29 Sep 2026 17:51:51 +0100 Subject: [PATCH 2/4] Test single-line ERB comments and say which todo entries go stale A single-line `<%# %>` comment already yielded no references on main, but nothing pinned it. `comment_contents_are_not_references` now includes one, and is renamed since it no longer covers only multi-line comments. It fails if a change strips the `#` instead of dropping the tag. The CHANGELOG now says the fix changes results in both directions. A todo entry recorded for a constant named inside a comment is no longer found, so `pks check` reports it as stale until `pks update` removes it. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 +++++- src/packs/parsing/erb/packwerk.rs | 3 ++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2beda7a2..e855050d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,11 @@ pks read every line of a multi-line `<%# %>` comment after the first as Ruby code. Depending on what the comment said, references elsewhere in the file were missed, sometimes all of them, and constants named inside the comment were -reported as references. ERB comments are now skipped, as packwerk does. +reported as references. ERB comments are now skipped, as packwerk does. This +changes results in both directions: references that were missed are now +reported, and a `package_todo.yml` entry recorded for a constant named inside a +comment is no longer found, so `pks check` reports it as stale until you run +`pks update`. pks also found nothing at all in an ERB file whose first tag was an encoding comment, such as `<%# encoding: iso-8859-1 %>`, unless it named UTF-8 or diff --git a/src/packs/parsing/erb/packwerk.rs b/src/packs/parsing/erb/packwerk.rs index 12a160f7..d6842428 100644 --- a/src/packs/parsing/erb/packwerk.rs +++ b/src/packs/parsing/erb/packwerk.rs @@ -279,9 +279,10 @@ mod tests { } #[test] - fn multiline_comment_contents_are_not_references() { + fn comment_contents_are_not_references() { let contents: String = String::from( "/ +<%# Qux %> <%# Foo %> From 41940a90d3adcb180cc5d1dc146380146c195e71 Mon Sep 17 00:00:00 2001 From: Mattia Giuffrida Date: Tue, 29 Sep 2026 17:52:29 +0100 Subject: [PATCH 3/4] Include the pks version in the cache's content digest A cache entry was reused whenever the file's content matched, whatever version of pks wrote it. So after an upgrade, a file that hadn't changed kept the result the old version produced, which hid parser fixes such as the ERB one until the user ran `pks delete-cache`. EmptyCacheEntry::new now appends CARGO_PKG_VERSION to the content digest. PerFileCache::get compares the stored digest with it, so the first run after an upgrade misses on every entry and rewrites it at the same path (the file name is the md5 of the file path). The digest isn't read anywhere else. The cost is one cold run per upgrade. The new test writes an entry, checks it's served, then replaces its digest with the bare content digest (what earlier versions stored) and with another version's, and checks both are misses. It fails without this change. The CHANGELOG's `pks delete-cache` paragraph is replaced by an entry for this. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 9 ++++-- src/packs/caching/mod.rs | 6 +++- src/packs/caching/per_file_cache.rs | 45 +++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e855050d..c886e649 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,9 +20,12 @@ comment, such as `<%# encoding: iso-8859-1 %>`, unless it named UTF-8 or ASCII-8BIT. Those files are now checked like any other, so they may show new violations. -Cached results aren't tied to the pks version, so an ERB file you haven't -changed keeps its old result after you upgrade. Unless you've turned the cache -off, run `pks delete-cache` once after upgrading so ERB files are read again. +#### Upgrading pks invalidates cached results + +Cached results didn't record which version of pks produced them, so after an +upgrade pks kept serving old results for files that hadn't changed, including +results the ERB fix above corrects. Each entry now includes the pks version, so +the first run after an upgrade re-reads every file. #### `check -o json` and `-o csv` list each strict violation once diff --git a/src/packs/caching/mod.rs b/src/packs/caching/mod.rs index c3485f06..c772eb42 100644 --- a/src/packs/caching/mod.rs +++ b/src/packs/caching/mod.rs @@ -29,7 +29,11 @@ impl EmptyCacheEntry { let file_name_digest = format!("{:x}", file_digest); let cache_file_path = cache_directory.join(&file_name_digest); - let file_contents_digest = file_content_digest(filepath)?; + let file_contents_digest = format!( + "{}-{}", + file_content_digest(filepath)?, + env!("CARGO_PKG_VERSION") + ); Ok(EmptyCacheEntry { filepath: filepath.to_owned(), diff --git a/src/packs/caching/per_file_cache.rs b/src/packs/caching/per_file_cache.rs index 8d4118b1..193cf0a9 100644 --- a/src/packs/caching/per_file_cache.rs +++ b/src/packs/caching/per_file_cache.rs @@ -222,4 +222,49 @@ mod tests { Ok(()) } + + #[test] + fn test_entry_from_another_pks_version_is_a_miss() -> anyhow::Result<()> { + let cache_dir = tempfile::tempdir()?; + let cache = PerFileCache { + cache_dir: cache_dir.path().to_path_buf(), + }; + let path = PathBuf::from( + "tests/fixtures/simple_app/packs/bar/app/services/bar.rb", + ); + let processed_file = ProcessedFile { + absolute_path: path.clone(), + unresolved_references: vec![], + definitions: vec![], + }; + + let CacheResult::Miss(empty_cache_entry) = cache.get(&path)? else { + panic!("expected a miss on an empty cache"); + }; + cache.write(&empty_cache_entry, &processed_file)?; + assert!(matches!(cache.get(&path)?, CacheResult::Processed(_))); + + // The bare content digest is what entries held before they recorded + // the pks version. + let content_digest = file_content_digest(&path)?; + for file_contents_digest in + [content_digest.clone(), format!("{content_digest}-0.0.0")] + { + let entry = CacheEntry { + file_contents_digest, + processed_file: processed_file.clone(), + }; + fs::write( + &empty_cache_entry.cache_file_path, + serde_json::to_string(&entry)?, + )?; + assert!( + matches!(cache.get(&path)?, CacheResult::Miss(_)), + "served an entry with digest {}", + entry.file_contents_digest + ); + } + + Ok(()) + } } From c8f4649905016c536d93e96d6b5b2648a5765e58 Mon Sep 17 00:00:00 2001 From: Mattia Giuffrida Date: Wed, 30 Sep 2026 09:41:35 +0100 Subject: [PATCH 4/4] Treat ERB's `<%%` literal as text `<%%` is ERB's escape for a literal `<%`. Erubi, which Rails renders with, outputs the whole tag up to the first `%>` as text: `<%%= x %>` renders `<%= x %>`, and nothing in it is evaluated. pks passed its contents on as Ruby, `%= x` or `% if x`, which broke the parse, so a file using it lost some or all of its references. Tags opening with `<%%` are now dropped before the join, like comments. `%%>` needs no handling. Erubi gives it no special meaning, and a tag ends at the first `%>` in Erubi and in pks, so `<%% x %%>` is the text `<% x %%>`. This is where pks deliberately stops matching packwerk. packwerk parses a literal's contents as Ruby along with the rest of the file, so a file with `<%%=` fails to parse, and other literals can report constants from inside them. pks follows Erubi, which evaluates none of it. The new test fails on the previous commit, which finds the reference before the literal and loses the one after it. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 9 ++++++++- src/packs/file_utils.rs | 4 +++- src/packs/parsing/erb/packwerk.rs | 33 +++++++++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c886e649..73ff3a45 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,11 +20,18 @@ comment, such as `<%# encoding: iso-8859-1 %>`, unless it named UTF-8 or ASCII-8BIT. Those files are now checked like any other, so they may show new violations. +#### `<%%` in ERB no longer hides references + +`<%%` writes a literal `<%`, but pks read the rest of the tag as Ruby, so a file +using it lost some or all of its references. It's now treated as text, as Rails +does. packwerk parses the literal as Ruby instead, so it fails on `<%%=` and can +report constants from inside other literals. + #### Upgrading pks invalidates cached results Cached results didn't record which version of pks produced them, so after an upgrade pks kept serving old results for files that hadn't changed, including -results the ERB fix above corrects. Each entry now includes the pks version, so +results the ERB fixes above correct. Each entry now includes the pks version, so the first run after an upgrade re-reads every file. #### `check -o json` and `-o csv` list each strict violation once diff --git a/src/packs/file_utils.rs b/src/packs/file_utils.rs index ba497912..929ddb6c 100644 --- a/src/packs/file_utils.rs +++ b/src/packs/file_utils.rs @@ -109,7 +109,9 @@ pub(crate) fn convert_erb_to_ruby_without_sourcemaps( let extracted_contents: Vec<&str> = regex .captures_iter(&contents) - .filter(|capture| !capture[0].starts_with("<%#")) + .filter(|capture| { + !capture[0].starts_with("<%#") && !capture[0].starts_with("<%%") + }) .map(|capture| capture.get(1).unwrap().as_str()) .collect(); diff --git a/src/packs/parsing/erb/packwerk.rs b/src/packs/parsing/erb/packwerk.rs index d6842428..ee5c8bbc 100644 --- a/src/packs/parsing/erb/packwerk.rs +++ b/src/packs/parsing/erb/packwerk.rs @@ -327,6 +327,39 @@ mod tests { ); } + #[test] + fn literal_tag_is_text_not_ruby() { + let contents: String = String::from( + "/ +<%= Foo %> +<%%= Qux %> +<%% if Quux %%> +<%= Bar %> + ", + ); + let configuration = Configuration::default(); + assert_eq!( + vec![ + UnresolvedReference { + name: String::from("Foo"), + namespace_path: vec![], + location: Range::default() + }, + UnresolvedReference { + name: String::from("Bar"), + namespace_path: vec![], + location: Range::default() + } + ], + process_from_contents( + contents, + &PathBuf::from("path/to/file.rb"), + &configuration + ) + .unresolved_references + ); + } + #[test] fn hash_after_hyphen_or_space_is_code_not_comment() { let contents: String = String::from(