tap_auditor: add comprehensive validation for formula and cask renames #21011

loganrosen · 2025-11-10T02:04:13Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew typecheck with your changes locally?
Have you successfully run brew tests with your changes locally?

Overview

This PR enhances TapAuditor to provide comprehensive validation for formula_renames.json and cask_renames.json files in taps, and updates the rename documentation.

Changes

New Validation Checks (`tap_auditor.rb`)

The new check_renames method validates rename entries and detects:

Invalid format: .rb file extensions in rename keys or values (should use tokens only)
Missing targets: Rename targets that don't exist in the tap
Chained renames: Detects rename chains (A → B → C → D) and suggests collapsing them to the final target (A → D). Includes cycle detection to prevent infinite loops.
Name conflicts: Old names that conflict with existing formulae/casks in the tap

Implementation Details

Added cask_renames to attr_reader (was missing)
Replaced simple check_formula_list call for formula_renames.json with check_renames method
Added check_renames call for cask_renames.json validation
Chained rename detection follows chains to their final target with cycle detection
Implemented comprehensive test coverage for all validation scenarios

Documentation Updates (`docs/Rename-A-Formula.md`)

Added missing cask rename process documentation
Added "Important Rules" section documenting all validation requirements
Updated title to "Renaming a Formula or Cask" to reflect complete coverage
Provided clear examples for both formula and cask renames

Motivation

This was motivated by Homebrew/homebrew-cask#235881, which fixes an incorrect cask_renames.json entry that erroneously included a .rb file extension in the old cask name. This caused Homebrew to fail to properly migrate users from multiviewer-for-f1 to multiviewer, breaking the rename functionality entirely.

Currently, there's no validation to catch these errors before they're merged, meaning broken renames can silently break user upgrades. This PR adds auditing to prevent such issues:

Invalid extensions: Catches .rb extensions that break rename matching
Chained renames: Detects and suggests collapsing multi-level chains (A → B → C → D becomes A → D)
Missing targets: Ensures renames point to formulae/casks that actually exist
Name conflicts: Warns when old names conflict with existing formulae/casks

These checks will catch issues during brew audit before they're merged and affect users.

Testing

All new functionality is covered by RSpec tests in tap_auditor_spec.rb:

Tests for cask rename validation (7 test cases, including multi-level chain detection)
Tests for formula rename validation (3 test cases)
All tests passing locally

Local validation:

✓ brew style (No offenses)
✓ brew typecheck (No errors)
✓ brew tests --only=tap_auditor (All 10 tests passing)

- Add check_renames method to validate rename entries - Detect .rb extensions in rename keys/values - Validate rename targets exist in tap - Detect and report chained renames - Warn about old names conflicting with existing formulae/casks - Add cask_renames to attr_reader - Add comprehensive test coverage for rename validation

Copilot

Pull Request Overview

This PR adds comprehensive validation for cask_renames.json and formula_renames.json files in Homebrew taps, replacing basic existence checking with detailed auditing.

Implements a new check_renames method that validates rename entries for both casks and formulae
Detects common issues: .rb extensions, missing targets, chained renames, and naming conflicts
Adds comprehensive test coverage for cask rename validation and partial coverage for formula renames

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
Library/Homebrew/tap_auditor.rb	Adds `cask_renames` attribute, implements `check_renames` method with four validation checks, and integrates validation into the audit process
Library/Homebrew/test/tap_auditor_spec.rb	Adds comprehensive test suite covering cask_renames validation scenarios and partial test coverage for formula_renames validation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Add missing cask rename process documentation - Document validation rules for both formula and cask renames - Explain .rb extension, chained renames, and other common issues - Update title to reflect coverage of both formulae and casks

Copilot

Pull Request Overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Library/Homebrew/tap_auditor.rb

… end - Follow rename chains to their final target (e.g., A → B → C → D now correctly suggests A → D) - Add cycle detection to prevent infinite loops on circular renames - Add test case for multi-level chained renames - Simplify suggestion message since chains can be arbitrarily long Addresses PR feedback from Homebrew#21011

Copilot

Pull Request Overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Library/Homebrew/tap_auditor.rb

MikeMcQuaid

@loganrosen Was an LLM used to generated any of the PR description and/or code here? If so, can you confirm you've manually reviewed, shortened and simplified it all as much as possible to the best of your ability and you understand it all? Thanks!

Copilot

Pull Request Overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

loganrosen · 2025-11-11T00:03:09Z

@loganrosen Was an LLM used to generated any of the PR description and/or code here? If so, can you confirm you've manually reviewed, shortened and simplified it all as much as possible to the best of your ability and you understand it all? Thanks!

@MikeMcQuaid Yes, I did use an LLM to assist with the PR description and code changes, but I've manually reviewed all of the information/changes and understand them fully. Your nudge did lead me to some slight improvements in the error messages, though. 🙂

MikeMcQuaid · 2025-11-12T15:50:58Z

docs/Rename-A-Formula.md

+## Important Rules
+
+When editing `formula_renames.json` or `cask_renames.json`, follow these rules:
+
+1. **Do not include `.rb` file extensions** - Use only the formula/cask token name
+2. **Ensure the target exists** - The new name must be an existing formula/cask in the tap
+3. **Avoid chained renames** - Don't create chains like `A → B` and `B → C`. Collapse chains to `A → C` directly as chained renames don't work automatically
+4. **Check for conflicts** - The old name should not conflict with an existing formula/cask
+
+These rules are validated by `brew audit` for tap maintainers.


I don't think it's necessary to duplicate what the tooling does in the documentation. They will become out of sync.

MikeMcQuaid · 2025-11-12T15:51:42Z

Library/Homebrew/test/tap_auditor_spec.rb

+        expect(auditor.problems.first[:message]).to include("'.rb' file extensions")
+        expect(auditor.problems.first[:message]).to include("oldcask.rb")


Do a full match on the message instead of two includes and an empty check.

This also applies to all the test cases below too.

MikeMcQuaid · 2025-11-12T15:52:16Z

Library/Homebrew/test/tap_auditor_spec.rb

+        cask_renames_path.write JSON.pretty_generate({
+          "oldcask.rb" => "newcask",
+        })
+
+        auditor.audit


Use before and subject to DRY this up

MikeMcQuaid · 2025-11-12T15:52:35Z

Library/Homebrew/tap_auditor.rb

      check_formula_list_directory "style_exceptions", @tap_style_exceptions
      check_formula_list "pypi_formula_mappings", @tap_pypi_formula_mappings
-      check_formula_list "formula_renames", @formula_renames.values
+      check_renames "formula_renames", @formula_renames, @formula_names, @formula_aliases


Feels weird passing the same argument twice

MikeMcQuaid · 2025-11-12T15:53:04Z

Library/Homebrew/tap_auditor.rb

+             valid_aliases: T::Array[String]).void
+    }
+    def check_renames(list_file, renames_hash, valid_tokens, valid_aliases = [])
+      list_file += ".json" if File.extname(list_file).empty?


This seems strange; why wouldn't you just check the passed filename?

MikeMcQuaid · 2025-11-12T15:55:35Z

Library/Homebrew/tap_auditor.rb

+        problem <<~EOS
+          #{list_file} contains entries with '.rb' file extensions.
+          Rename entries should use formula/cask names only, without '.rb' extensions.
+          Invalid entries: #{invalid_format.map { |k, v| "\"#{k}\": \"#{v}\"" }.join(", ")}


Use filter_map instead of select above and make it a single array that can be .join(", ") here instead.

MikeMcQuaid · 2025-11-12T15:56:53Z

Library/Homebrew/tap_auditor.rb

+      if chained.any?
+        suggestions = chained.map do |old_name, intermediate|
+          # Follow the chain to the end, with cycle detection
+          final = intermediate
+          seen = Set.new([old_name, intermediate])
+          while renames_hash.key?(final)
+            next_name = renames_hash[final]
+            break if next_name.nil? || seen.include?(next_name) # Prevent infinite loops on circular renames
+
+            final = next_name
+            seen << final
+          end
+          "  \"#{old_name}\": \"#{final}\" (instead of chained rename)"
+        end


This seems unnecessarily complex and doesn't need a while. Instead you can just see if old_names contains any new_names with a single array intersection operation.

MikeMcQuaid · 2025-11-12T15:57:49Z

Library/Homebrew/tap_auditor.rb

+      end
+
+      # Warn if old names conflict with existing casks/formulae
+      conflicts = renames_hash.keys.select { |old_name| valid_tokens.include?(old_name) }


It might be nicer for readability to just loop through renames_hash once and append to several arrays inside that single loop instead of looping for each check.

Copilot AI review requested due to automatic review settings November 10, 2025 02:04

Copilot AI reviewed Nov 10, 2025

View reviewed changes

loganrosen requested a review from Copilot November 10, 2025 02:14

Copilot AI reviewed Nov 10, 2025

View reviewed changes

Library/Homebrew/tap_auditor.rb Show resolved Hide resolved

loganrosen requested a review from Copilot November 10, 2025 02:28

Copilot AI reviewed Nov 10, 2025

View reviewed changes

Library/Homebrew/tap_auditor.rb Outdated Show resolved Hide resolved

Library/Homebrew/tap_auditor.rb Show resolved Hide resolved

MikeMcQuaid reviewed Nov 10, 2025

View reviewed changes

loganrosen added 2 commits November 10, 2025 18:44

Merge branch 'main' into tap-auditor-rename-validation

24fd72d

Error message improvements

2e67d94

Copilot AI review requested due to automatic review settings November 10, 2025 23:58

Copilot started reviewing on behalf of loganrosen November 10, 2025 23:58 View session

Copilot finished reviewing on behalf of loganrosen November 11, 2025 00:01

Copilot AI reviewed Nov 11, 2025

View reviewed changes

loganrosen marked this pull request as ready for review November 11, 2025 00:14

MikeMcQuaid reviewed Nov 12, 2025

View reviewed changes

		expect(auditor.problems.first[:message]).to include("'.rb' file extensions")
		expect(auditor.problems.first[:message]).to include("oldcask.rb")

Uh oh!

tap_auditor: add comprehensive validation for formula and cask renames #21011

Are you sure you want to change the base?

tap_auditor: add comprehensive validation for formula and cask renames #21011

Conversation

loganrosen commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Changes

New Validation Checks (tap_auditor.rb)

Implementation Details

Documentation Updates (docs/Rename-A-Formula.md)

Motivation

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

loganrosen commented Nov 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

loganrosen commented Nov 10, 2025 •

edited

Loading

New Validation Checks (`tap_auditor.rb`)

Documentation Updates (`docs/Rename-A-Formula.md`)