[Bug Report] Getting "There were stale violations found, please run packwerk update-todo" even there is no stale violations

[kalys]

Description
I'm getting "There were stale violations found, please run packwerk update-todo" when I run check command againts some files.
For this you have to have one violation in several files.

For example:

# This file contains a list of dependencies that are not part of the long term plan for the
# 'packs/one' package.
# We should generally work to reduce this list over time.
#
# You can regenerate this file using the following command:
#
# bin/packwerk update-todo
---
".":
  "::ApplicationController":
    violations:
    - dependency
    files:
    - packs/one/bar.rb
    - packs/one/baz.rb

To Reproduce

1. Create a dependency violation in the first file.
2. Create the same dependency violation in another file within the same package.
3. Run packwerk update-todo in order to generate package_todo.yml file.
4. Run packwerk check path/to/the/first_file.rb.

Actual Behaviour

• Command returns 1 status.
• Command output contains info about stale violations.

📦 Finished in X.XX seconds

No offenses detected
There were stale violations found, please run `packwerk update-todo`

Expected Behaviour

• Command returns 0 status
• Command output has no info about stale violations

Version Information

• Packwerk: 3.1.0
• Ruby: 3.2.1

Additional Context
3.0.1 has no this problem

[rafaelfranca]

What happen if you run update-todo?

[kalys]

@rafaelfranca I added package update-todo step to STR.

[krisleech]

I know this isn't much help, but we had this problem on CI for a day or so, but it just went away. Running update-todo did not help.

[jmcfarland-figma]

We have this problem when running packwerk check FILE_NAME to check a single file. When compiling the list of old_violations inside the PackageTodo class it looks like Packwerk is not correctly limiting the scope to the file being checked.

Then when it gets to this line:

# lib/packwerk/package_todo.rb:134
new_entries_violation_types = new_entries.dig(package, constant_name, "violations")

There are no new entries for the package, and the logic assumes this means there are stale entries. This makes sense when checking an entire codebase, but not when checking a single file or a group of files.

Running packwerk update-todo doesn't help because there are no stale violations to fix, and packwerk update-todo raises an error if you supply a single file as an argument anyway.

[jmcfarland-figma]

@rafaelfranca I have a failing test for PackageTodo.

    test "#stale_violations? returns false when violations in package_todo.yml exist but aren't relevant to passed in files" do
      package = Package.new(name: "buyers", config: { "enforce_dependencies" => true })
      violated_reference = build_reference(
        destination_package: package,
        path: "some/other/file.rb",
        constant_name: "::Buyers::Document"
      )
      package_todo = PackageTodo.new(package, "test/fixtures/package_todo.yml")
      package_todo.add_entries(violated_reference, "some_checker_type")
      refute package_todo.stale_violations?(
        Set.new(["some/other/file.rb"])
      )
    end

I believe the issue comes down to deleted_files_for - it does new_entries - old_entries to look for references that are no longer valid, but this doesn't work in the case where you pass in a single file.

For reference, we do this as a pre-commit hook, we run packwerk check $(git diff --name-only) so that developers have a fast-fail way to know if their changes will break CI.

I'm not sure what the right fix is; happy to take some direction and try to put together a patch tho.

[jmcfarland-figma]

Okay, I actually don't like the failing test I shared above. It seems like the problem sits with the deleted_files_for - it's passed a Set of files with no context on whether or not the files are all the files for the project or the files passed in via the CLI.

The object that holds that information is the FilesForProcessing object that has files and files_specified?, but by the time PackageTodo calls deleted_files_for it doesn't have the FilesForProcessing references anymore, just FilesForProcessing#files which is the Set.

I'll try to cook up a PR that passes the FilesForProcessing object all the way through.

[jmcfarland-figma]

This is the shape of the diff I came up with. I need to clean up tests before I submit a PR

diff --git a/lib/packwerk/commands/check_command.rb b/lib/packwerk/commands/check_command.rb
index 43d9608..2c7b770 100644
--- a/lib/packwerk/commands/check_command.rb
+++ b/lib/packwerk/commands/check_command.rb
@@ -35,14 +35,14 @@ module Packwerk

         messages = [
           offenses_formatter.show_offenses(offense_collection.outstanding_offenses),
-          offenses_formatter.show_stale_violations(offense_collection, @files_for_processing.files),
+          offenses_formatter.show_stale_violations(offense_collection, @files_for_processing),
           offenses_formatter.show_strict_mode_violations(unlisted_strict_mode_violations),
         ]

         out.puts(messages.select(&:present?).join("\n") + "\n")

         offense_collection.outstanding_offenses.empty? &&
-          !offense_collection.stale_violations?(@files_for_processing.files) &&
+          !offense_collection.stale_violations?(@files_for_processing) &&
           unlisted_strict_mode_violations.empty?
       end

diff --git a/lib/packwerk/formatters/default_offenses_formatter.rb b/lib/packwerk/formatters/default_offenses_formatter.rb
index 6f12aba..9a683f1 100644
--- a/lib/packwerk/formatters/default_offenses_formatter.rb
+++ b/lib/packwerk/formatters/default_offenses_formatter.rb
@@ -20,9 +20,9 @@ module Packwerk
         EOS
       end

-      sig { override.params(offense_collection: OffenseCollection, file_set: T::Set[String]).returns(String) }
-      def show_stale_violations(offense_collection, file_set)
-        if offense_collection.stale_violations?(file_set)
+      sig { override.params(offense_collection: OffenseCollection, files_for_processing: FilesForProcessing).returns(String) }
+      def show_stale_violations(offense_collection, files_for_processing)
+        if offense_collection.stale_violations?(files_for_processing)
           "There were stale violations found, please run `packwerk update-todo`"
         else
           "No stale violations detected"
diff --git a/lib/packwerk/offense_collection.rb b/lib/packwerk/offense_collection.rb
index 1e6d638..27f4135 100644
--- a/lib/packwerk/offense_collection.rb
+++ b/lib/packwerk/offense_collection.rb
@@ -67,10 +67,10 @@ module Packwerk
       end
     end

-    sig { params(for_files: T::Set[String]).returns(T::Boolean) }
-    def stale_violations?(for_files)
+    sig { params(files_for_processing: FilesForProcessing).returns(T::Boolean) }
+    def stale_violations?(files_for_processing)
       @package_todos.values.any? do |package_todo|
-        package_todo.stale_violations?(for_files)
+        package_todo.stale_violations?(files_for_processing)
       end
     end

diff --git a/lib/packwerk/package_todo.rb b/lib/packwerk/package_todo.rb
index 4eecd24..f6eaf69 100644
--- a/lib/packwerk/package_todo.rb
+++ b/lib/packwerk/package_todo.rb
@@ -54,12 +54,16 @@ module Packwerk
       listed?(reference, violation_type: violation_type)
     end

-    sig { params(for_files: T::Set[String]).returns(T::Boolean) }
-    def stale_violations?(for_files)
+    sig { params(files_for_processing: FilesForProcessing).returns(T::Boolean) }
+    def stale_violations?(files_for_processing)
       prepare_entries_for_dump

+      for_files = files_for_processing.files
+
       old_entries.any? do |package, violations|
-        files = for_files + deleted_files_for(package)
+        # We don't try to detect deleted files when files were specified on the CLI because
+        # we don't know if the file was deleted or just not specified
+        files = files_for_processing.specified_files? ? for_files : for_files + deleted_files_for(package)
         violations_for_files = package_violations_for(violations, files: files)