From 26555c6f05ca7962275944d89ec6c3026f8736a3 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 11 Aug 2026 13:14:49 -0700 Subject: [PATCH] Fix the declared return type of RustCodeOwners.for_file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Rust extension serializes a `Team` struct with three fields — `team_name` and `team_config_yml` (both `String`) plus `reasons` (`Vec`) — so the `T::Hash[Symbol, String]` declared for it in `TeamFinder.for_file` and `CodeOwnership.for_file_verbose` never matched: `reasons` is an Array. Sorbet's runtime only checks the outer class of a collection type, so `T::Hash[Symbol, String]` was satisfied by any Hash and the mismatch went unnoticed. It surfaces as soon as a host application makes `valid?` descend into elements (`recursively_valid?`), where every `for_file` call raises. Replace both with a shape matching the struct. `team_name` is a non-optional Rust `String`, so it can never serialize to nil: the `result[:team_name].nil?` branch and its paired `T.must` are now dead, and the stubs in the specs return the whole struct rather than a `team_name`-only hash. --- lib/code_ownership.rb | 7 ++++++- lib/code_ownership/private/team_finder.rb | 6 +++--- lib/code_ownership/version.rb | 2 +- .../code_ownership/private/team_finder_spec.rb | 16 ++++++---------- spec/lib/code_ownership_spec.rb | 3 ++- 5 files changed, 18 insertions(+), 16 deletions(-) diff --git a/lib/code_ownership.rb b/lib/code_ownership.rb index a78707d..67f666b 100644 --- a/lib/code_ownership.rb +++ b/lib/code_ownership.rb @@ -28,6 +28,11 @@ module CodeOwnership GlobsToOwningTeamMap = T.type_alias { T::Hash[String, CodeTeams::Team] } + # The `Team` struct that the Rust extension serializes out of `RustCodeOwners.for_file`. + FileOwnershipDetails = T.type_alias do + { team_name: String, team_config_yml: String, reasons: T::Array[String] } + end + # Returns the version of the code_ownership gem and the codeowners-rs gem. sig { returns(T::Array[String]) } def self.version @@ -154,7 +159,7 @@ def self.teams_for_files_from_codeowners(files, allow_raise: false) # @see #for_file for a simpler ownership lookup that returns just the team # @see CLI#for_file for the command-line interface that uses this method # - sig { params(file: String).returns(T.nilable(T::Hash[Symbol, String])) } + sig { params(file: String).returns(T.nilable(FileOwnershipDetails)) } def self.for_file_verbose(file) ::RustCodeOwners.for_file(file) end diff --git a/lib/code_ownership/private/team_finder.rb b/lib/code_ownership/private/team_finder.rb index 9f26e16..3a035f1 100644 --- a/lib/code_ownership/private/team_finder.rb +++ b/lib/code_ownership/private/team_finder.rb @@ -12,12 +12,12 @@ def self.for_file(file_path, allow_raise: false) return FilePathTeamCache.get(file_path) if FilePathTeamCache.cached?(file_path) - result = T.let(RustCodeOwners.for_file(file_path), T.nilable(T::Hash[Symbol, String])) + result = T.let(RustCodeOwners.for_file(file_path), T.nilable(FileOwnershipDetails)) - if result.nil? || result[:team_name].nil? + if result.nil? FilePathTeamCache.set(file_path, nil) else - FilePathTeamCache.set(file_path, T.let(find_team!(T.must(result[:team_name]), allow_raise: allow_raise), T.nilable(CodeTeams::Team))) + FilePathTeamCache.set(file_path, T.let(find_team!(result[:team_name], allow_raise: allow_raise), T.nilable(CodeTeams::Team))) end FilePathTeamCache.get(file_path) diff --git a/lib/code_ownership/version.rb b/lib/code_ownership/version.rb index 9801eec..7815856 100644 --- a/lib/code_ownership/version.rb +++ b/lib/code_ownership/version.rb @@ -2,5 +2,5 @@ # frozen_string_literal: true module CodeOwnership - VERSION = '2.1.3' + VERSION = '2.1.4' end diff --git a/spec/lib/code_ownership/private/team_finder_spec.rb b/spec/lib/code_ownership/private/team_finder_spec.rb index d4dac78..d325297 100644 --- a/spec/lib/code_ownership/private/team_finder_spec.rb +++ b/spec/lib/code_ownership/private/team_finder_spec.rb @@ -4,13 +4,17 @@ describe '.for_file' do let(:file_path) { 'packs/my_pack/owned_file.rb' } + let(:rust_result) do + { team_name: 'Bar', team_config_yml: 'config/teams/bar.yml', reasons: [] } + end + before do create_non_empty_application end it 'caches positive results' do allow(RustCodeOwners).to receive(:for_file).with(file_path) - .and_return({ team_name: 'Bar' }, nil) + .and_return(rust_result, nil) first = described_class.for_file(file_path) second = described_class.for_file(file_path) @@ -22,7 +26,7 @@ it 'caches nil when rust returns nil' do allow(RustCodeOwners).to receive(:for_file).with(file_path) - .and_return(nil, { team_name: 'Bar' }) + .and_return(nil, rust_result) first = described_class.for_file(file_path) second = described_class.for_file(file_path) @@ -31,14 +35,6 @@ expect(second).to be_nil expect(CodeOwnership::Private::FilePathTeamCache.cached?(file_path)).to be true end - - it 'caches nil when team_name is nil' do - allow(RustCodeOwners).to receive(:for_file).with(file_path).and_return({ team_name: nil }) - - expect(described_class.for_file(file_path)).to be_nil - expect(CodeOwnership::Private::FilePathTeamCache.cached?(file_path)).to be true - expect(CodeOwnership::Private::FilePathTeamCache.get(file_path)).to be_nil - end end describe '.for_backtrace' do diff --git a/spec/lib/code_ownership_spec.rb b/spec/lib/code_ownership_spec.rb index f0cf596..0ec059c 100644 --- a/spec/lib/code_ownership_spec.rb +++ b/spec/lib/code_ownership_spec.rb @@ -215,7 +215,8 @@ end it 'raises an error when using single-file path' do - allow(RustCodeOwners).to receive(:for_file).and_return({ team_name: 'Made Up Team' }) + allow(RustCodeOwners).to receive(:for_file) + .and_return({ team_name: 'Made Up Team', team_config_yml: 'config/teams/made_up_team.yml', reasons: [] }) expect { CodeOwnership.for_file(file_path, from_codeowners: false, allow_raise: true) }.to raise_error(StandardError, /Could not find team with name:/) end end