From 6ce590f5aa2a9300c30720bf85e0d446e2f6aca5 Mon Sep 17 00:00:00 2001 From: OdenTakashi Date: Fri, 31 Jul 2026 00:43:07 +0900 Subject: [PATCH] Stop same-named models in different packs from annotating each other's related files by matching on owning root_dir. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Prevent related-file collisions between models with the same basename in different roots. ## Problem In a Packwerk application with: * `app/models/address.rb` → `Address` (table: `addresses`) * `packs/contacts/app/public/models/address.rb` → `Contacts::Address` (table: `contact_addresses`) * `packs/contacts/test/models/address_test.rb` → test for `Contacts::Address` both models claim the same test file as a related file. Each model writes its own schema annotation to it—`addresses` or `contact_addresses`—and the last writer wins. As a result, a normal run reports the same file twice: ```sh Annotated (2): packs/contacts/test/models/address_test.rb, packs/contacts/test/models/address_test.rb ``` `--frozen` then fails for whichever model lost the last-write race, even though the annotations are not actually stale. ## Root cause With `root_dir: ["", "packs/*"]`, `PatternGetter` expands related-file patterns for every configured root, rather than only for the concrete root containing the current model. For both models, `%MODEL_NAME%` and `%MODEL_NAME_WITHOUT_NS%` resolve to `address`. This causes both models to generate and search the same glob, such as: ```text packs/*/test/models/address_test.rb ``` Consequently, both models match `packs/contacts/test/models/address_test.rb`. ## Solution After finding related files through the existing name-based globbing, filter them so that their concrete root matches the model's root: * `app/models/address.rb` belongs to the project root (`nil`) * It may match project-root related files such as `test/models/address_test.rb` * It does not match files under `packs/contacts` * `packs/contacts/app/public/models/address.rb` belongs to `packs/contacts` * It matches `packs/contacts/test/models/address_test.rb` Files in the project root continue to match each other because both have a root of `nil`. The default configuration, `root_dir: [""]`, is unchanged because all model and related files belong to the project root. Fixes #367 --- .../related_files_list_builder.rb | 29 ++++++++++++- .../related_files_list_builder_spec.rb | 43 +++++++++++++++++++ 2 files changed, 70 insertions(+), 2 deletions(-) diff --git a/lib/annotate_rb/model_annotator/related_files_list_builder.rb b/lib/annotate_rb/model_annotator/related_files_list_builder.rb index 0b6024a9..d61f5d3f 100644 --- a/lib/annotate_rb/model_annotator/related_files_list_builder.rb +++ b/lib/annotate_rb/model_annotator/related_files_list_builder.rb @@ -88,8 +88,33 @@ def related_files_for_pattern(pattern_type) patterns .map { |f| FileNameResolver.call(f, @model_name, @table_name) } - .map { |f| Dir.glob(f) } - .flatten + .flat_map { |f| Dir.glob(f) } + .select { |f| owning_root_dir(f) == model_root_dir } + end + + def model_root_dir + return @model_root_dir if defined?(@model_root_dir) + + @model_root_dir = owning_root_dir(@file) + end + + # Patterns are expanded for every `root_dir` and resolved by model name alone, so models sharing a + # file basename across root directories (e.g. packwerk packs) glob the same related files. Root + # directories can be nested, so a file belongs to the most specific one containing it, and only + # models from that same root directory may annotate it. Returns nil for the project root. + def owning_root_dir(file) + expanded_file = File.expand_path(file) + + expanded_root_dirs + .select { |dir| expanded_file.start_with?("#{dir}/") } + .max_by(&:length) + end + + def expanded_root_dirs + @expanded_root_dirs ||= Array(@options[:root_dir]) + .reject { |root_dir| root_dir.to_s.empty? } + .flat_map { |root_dir| Dir.glob(root_dir) } + .map { |root_dir| File.expand_path(root_dir) } end def add_related_test_files diff --git a/spec/lib/annotate_rb/model_annotator/related_files_list_builder_spec.rb b/spec/lib/annotate_rb/model_annotator/related_files_list_builder_spec.rb index f86b2cff..6b198779 100644 --- a/spec/lib/annotate_rb/model_annotator/related_files_list_builder_spec.rb +++ b/spec/lib/annotate_rb/model_annotator/related_files_list_builder_spec.rb @@ -441,5 +441,48 @@ expect(subject).to eq([[relative_file_path, position_key]]) end end + + context "when models in different root directories share a file basename", :isolated_environment do + let(:options) do + AnnotateRb::Options.new(**include_nothing_options.merge( + { + exclude_tests: false, + root_dir: ["", "packs/*"], + additional_file_patterns: ["packs/*/test/models/%MODEL_NAME_WITHOUT_NS%_test.rb"] + } + )) + end + + let(:root_test_file) { "test/models/address_test.rb" } + let(:pack_test_file) { "packs/contacts/test/models/address_test.rb" } + + before do + FileUtils.mkdir_p("test/models") + FileUtils.touch(root_test_file) + + FileUtils.mkdir_p("packs/contacts/test/models") + FileUtils.touch(pack_test_file) + end + + context "when the model is in the project root" do + let(:file) { "app/models/address.rb" } + let(:model_name) { "address" } + let(:table_name) { "addresses" } + + it "returns only the test file in the project root" do + expect(subject).to eq([[root_test_file, :position_in_test]]) + end + end + + context "when the model is in a pack" do + let(:file) { "packs/contacts/app/public/models/address.rb" } + let(:model_name) { "contacts/address" } + let(:table_name) { "contact_addresses" } + + it "returns only the test file in its own pack" do + expect(subject).to eq([[pack_test_file, :position_in_additional_file_patterns]]) + end + end + end end end