style: root the RuboCop project index at the tap - #23835
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
馃數 Needs a closer look
Mixed readiness assessments and unresolved guideline nits warrant final human review.
Pull request overview
Scopes RuboCop indexing to a single tap, preventing false cross-tap offenses while preserving intra-tap detection.
Changes:
- Adds a tap-specific RuboCop configuration.
- Selects tap-scoped configuration and working directory.
- Tests single- and multi-tap dispatch.
File summaries
| File | Description |
|---|---|
Library/tap_rubocop_style.yml |
Inherits shared RuboCop settings. |
Library/Homebrew/test/style_spec.rb |
Tests configuration dispatch. |
Library/Homebrew/style.rb |
Selects tap-scoped RuboCop indexing. |
Review details
Suppressed comments (1)
Library/Homebrew/test/style_spec.rb:289
- This repeats the multiple-expectation pattern prohibited for simple unit examples by
AGENTS.md:58. Please move both keyword-argument checks into.with(...), leaving this example with one expectation.
expect(described_class).to receive(:system_command) do |_cmd, args:, chdir:, **|
expect(args).to include("--config", HOMEBREW_LIBRARY/".rubocop.yml")
expect(chdir).to eq(HOMEBREW_LIBRARY)
result
end
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
馃挕 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
RuboCop roots its project index at the directory of a .rubocop* config file and at the working directory for any other name. brew style forces Library/.rubocop.yml for tap targets, so the index spanned every installed tap and Lint/DuplicateMethods and Lint/ConstantReassignment paired one tap's methods and constants with another's. A tap is now checked from its own directory through a differently named config that inherits the shared one, so the index covers that tap alone. Paths are passed as given so the shared config's Taps exclusions keep matching a symlinked tap. Signed-off-by: Patrick Linnane <patrick@linnane.io>
p-linnane
force-pushed
the
style-tap-project-index
branch
from
September 6, 2026 00:58
17db96c to
e0eb4c1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
brew styleon a tap reportsLint/DuplicateMethodsandLint/ConstantReassignmentoffenses that pair the tap's methods and constants with identically named ones in a different installed tap. Taps are never loaded together, so these are false positives, and they makebrew stylefail on a tap whose Ruby happens to share names with any other tap on the machine.The cause is where RuboCop roots its project index. RuboCop roots it at the directory of a
.rubocop*config file and at the working directory for any other config name.brew styleforcesLibrary/.rubocop.ymlfor tap targets, so the index is rooted atLibraryand spans every installed tap, and the cross-file cops compare across all of them. The shared config excludes only a tap's top-level*.rbfrom those cops, so anything undercmd/,scripts/,test/and similar collides.This change checks a tap from its own directory through
Library/tap_rubocop_style.yml, a config that only inherits the shared one. Because its name does not start with.rubocop, RuboCop roots the index at the working directory, which is the tap, so the index covers that tap alone. Cross-file detection inside the tap still works. Files spanning several taps, Homebrew's own code, and docs keep their current handling. Paths are passed as given rather than realpathed so the shared config'sTaps/...exclusions keep matching a tap that is a symlink into a checkout elsewhere.To reproduce on
main, install two copies of a tap that has Ruby below its top level, then style either of them:On
mainthis reports 127 offenses, every one naming a file in the other copy. With this change both copies are clean, a duplicate added inside one copy'sscripts/is still reported, and a duplicate hidden in a file that was not passed tobrew styleis still reported from a single-file run, which shows the index is rooted at the tap rather than absent.Two specs cover the dispatch: a single-tap target gets the tap config and the tap as working directory; files spanning two taps fall back to the shared config.
brew benchmarkresults.brewcommands to reproduce the bug?brew lgtm(style, typechecking and tests) locally?AI/LLM disclosure: Claude Fable 5.1 drafted the change, the config file and the specs under my direction, and I reviewed all of it. Verified with
brew lgtm,brew tests --only=style,brew typecheck, and by reproducing the false positive and each of the three behaviours above against two installed copies of a tap.