From ba0451d67fd4a7fbf234f8c08e9db30b51289324 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 20 Oct 2026 20:05:54 -0700 Subject: [PATCH 1/3] Fix inverted stage_changes in CodeOwnership.validate! validate! is documented to stage the regenerated .github/CODEOWNERS by default and to leave it unstaged with stage_changes: false, which is what bin/codeownership validate --skip-stage passes. It did the opposite. validate! passed !stage_changes to the extension, whose generate_and_validate took it as skip_stage, as the RBI declared, but handed it unchanged to codeowners-rs's runner::generate_and_validate, whose third parameter is git_stage. That has been the case since the extension was added in #126: the codeowners-rs it pinned already took git_stage (rubyatscale/codeowners-rs#65). codeowners-rs ignores the result of its git add, so nothing ever failed. Pass stage_changes through, and rename the extension's parameter and the RBI's to git_stage to match codeowners-rs, so the flag means the same thing at every layer. The extension's behavior is unchanged, so the tracked bundle needs no rebuild: the fix works with it as committed, and the specs that call RustCodeOwners.generate_and_validate(nil, false) to write a CODEOWNERS file still don't stage it. Add specs that validate! stages CODEOWNERS by default in a git repository and leaves it unstaged with stage_changes: false. Both fail without the fix. They clear git's repo-local environment variables first, so a run from a hook or a linked worktree can't stage into the enclosing repository. The CLI specs only check the arguments that reach validate!, so nothing covered what it does to the index. Add a regression guard for an empty package.yml. rubyatscale/codeowners-rs#142 moves YAML parsing to serde-saphyr, whose unreleased 2.1.0 no longer reads a null document as an empty struct (the fix for bourumir-wyngs/serde-saphyr#233). That PR reads package.yml as an Option so an empty file still means no owner. The spec covers an empty file, a comment-only file and a bare "---". validate! raises if such a file fails to parse, while for_file only prints the error and falls back to the file's annotation, so the spec also checks that for_file prints nothing. It passes today. --- ext/code_ownership/src/lib.rs | 4 +-- lib/code_ownership.rb | 2 +- sorbet/rbi/manual.rbi | 2 +- spec/lib/code_ownership_spec.rb | 52 +++++++++++++++++++++++++++++++++ 4 files changed, 56 insertions(+), 4 deletions(-) diff --git a/ext/code_ownership/src/lib.rs b/ext/code_ownership/src/lib.rs index 95e160b..da617ad 100644 --- a/ext/code_ownership/src/lib.rs +++ b/ext/code_ownership/src/lib.rs @@ -85,10 +85,10 @@ fn validate(ruby: &Ruby, files: Option>) -> Result { validate_result(ruby, &run_result) } -fn generate_and_validate(ruby: &Ruby, files: Option>, skip_stage: bool) -> Result { +fn generate_and_validate(ruby: &Ruby, files: Option>, git_stage: bool) -> Result { let run_config = build_run_config(); let files_vec = files.unwrap_or_default(); - let run_result = runner::generate_and_validate(&run_config, files_vec, skip_stage); + let run_result = runner::generate_and_validate(&run_config, files_vec, git_stage); validate_result(ruby, &run_result) } diff --git a/lib/code_ownership.rb b/lib/code_ownership.rb index 80bc414..31b4614 100644 --- a/lib/code_ownership.rb +++ b/lib/code_ownership.rb @@ -232,7 +232,7 @@ def self.validate!( files: nil ) if autocorrect - ::RustCodeOwners.generate_and_validate(files, !stage_changes) + ::RustCodeOwners.generate_and_validate(files, stage_changes) else ::RustCodeOwners.validate(files) end diff --git a/sorbet/rbi/manual.rbi b/sorbet/rbi/manual.rbi index feba3a1..6fe4e54 100644 --- a/sorbet/rbi/manual.rbi +++ b/sorbet/rbi/manual.rbi @@ -9,7 +9,7 @@ module RustCodeOwners def for_file(file) end - def generate_and_validate(files, skip_stage) + def generate_and_validate(files, git_stage) end def validate(files) diff --git a/spec/lib/code_ownership_spec.rb b/spec/lib/code_ownership_spec.rb index f1b46d7..b4d3bf8 100644 --- a/spec/lib/code_ownership_spec.rb +++ b/spec/lib/code_ownership_spec.rb @@ -620,6 +620,58 @@ expect { CodeOwnership.validate!(autocorrect: false) }.not_to raise_error end end + + context 'when the project is a git repository' do + let(:staged_files) { `git diff --cached --name-only`.lines(chomp: true) } + + # Hooks and linked worktrees export variables such as GIT_DIR and GIT_INDEX_FILE, which would aim git, + # including the extension's git add, at the repository running the suite instead of the temp project. + around do |example| + saved = ENV.to_h.slice(*`git rev-parse --local-env-vars`.split) + saved.each_key { |key| ENV.delete(key) } + example.run + ENV.update(saved) + end + + before do + create_non_empty_application + system('git', 'init', '--quiet', exception: true) + end + + it 'stages the CODEOWNERS file by default' do + CodeOwnership.validate! + expect(staged_files).to eq ['.github/CODEOWNERS'] + end + + it 'does not stage the CODEOWNERS file when stage_changes is false' do + CodeOwnership.validate!(stage_changes: false) + expect(codeowners_path).to exist + expect(staged_files).to be_empty + end + end + + ['', "# no keys yet\n", "---\n"].each do |contents| + context "when a package.yml is #{contents.inspect}" do + before do + write_configuration(owned_globs: ['packs/**/*.rb']) + write_file('config/teams/bar.yml', <<~CONTENTS) + name: Bar + github: + team: '@MyOrg/bar-team' + CONTENTS + write_file('packs/empty_pack/package.yml', contents) + write_file('packs/empty_pack/owned_file.rb', "# @team Bar\nclass OwnedFile; end\n") + end + + it 'reads it as a package with no owner' do + expect { CodeOwnership.validate!(stage_changes: false) }.not_to raise_error + # for_file falls back to the annotation when package.yml fails to parse, and only prints the error. + expect do + expect(CodeOwnership.for_file('packs/empty_pack/owned_file.rb', from_codeowners: false)).to eq CodeTeams.find('Bar') + end.not_to output.to_stderr_from_any_process + end + end + end end describe '.bust_caches!' do From a17ddd6fa49d3e68df44ff9eb3b55b1505293893 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Wed, 21 Oct 2026 09:28:43 -0700 Subject: [PATCH 2/3] Bump version to 2.2.1 --- lib/code_ownership/version.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/code_ownership/version.rb b/lib/code_ownership/version.rb index 667e740..4deded7 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.2.0' + VERSION = '2.2.1' end From e19e108e58316ff9cce73cd311bb6b18834672c6 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Wed, 21 Oct 2026 09:54:13 -0700 Subject: [PATCH 3/3] Clear git's repo-local env vars for the whole suite The fix makes every real validate! call with default arguments run git add, not only the new staging specs. Two cli_spec examples do that, and under a hook's GIT_DIR they staged .github/CODEOWNERS into the enclosing repository, since the scrub only wrapped the staging context. Move it to a before(:suite) hook so every example's git runs against its own temp project. Split the empty package.yml example in two, so validation and the for_file parse-error check pass or fail separately, without nesting one expect inside another. --- spec/lib/code_ownership_spec.rb | 21 ++++++++------------- spec/spec_helper.rb | 6 ++++++ 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/spec/lib/code_ownership_spec.rb b/spec/lib/code_ownership_spec.rb index b4d3bf8..ed2b5e4 100644 --- a/spec/lib/code_ownership_spec.rb +++ b/spec/lib/code_ownership_spec.rb @@ -624,15 +624,6 @@ context 'when the project is a git repository' do let(:staged_files) { `git diff --cached --name-only`.lines(chomp: true) } - # Hooks and linked worktrees export variables such as GIT_DIR and GIT_INDEX_FILE, which would aim git, - # including the extension's git add, at the repository running the suite instead of the temp project. - around do |example| - saved = ENV.to_h.slice(*`git rev-parse --local-env-vars`.split) - saved.each_key { |key| ENV.delete(key) } - example.run - ENV.update(saved) - end - before do create_non_empty_application system('git', 'init', '--quiet', exception: true) @@ -663,12 +654,16 @@ write_file('packs/empty_pack/owned_file.rb', "# @team Bar\nclass OwnedFile; end\n") end - it 'reads it as a package with no owner' do + it 'passes validation' do expect { CodeOwnership.validate!(stage_changes: false) }.not_to raise_error + end + + it 'leaves ownership to the annotation without printing a parse error' do + owner = nil # for_file falls back to the annotation when package.yml fails to parse, and only prints the error. - expect do - expect(CodeOwnership.for_file('packs/empty_pack/owned_file.rb', from_codeowners: false)).to eq CodeTeams.find('Bar') - end.not_to output.to_stderr_from_any_process + expect { owner = CodeOwnership.for_file('packs/empty_pack/owned_file.rb', from_codeowners: false) } + .not_to output.to_stderr_from_any_process + expect(owner).to eq CodeTeams.find('Bar') end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 56e2b39..52ae89b 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -21,6 +21,12 @@ config.include_context 'application fixtures' + # Hooks and linked worktrees export variables such as GIT_DIR and GIT_INDEX_FILE, which would aim git, including + # the extension's git add in validate!, at the repository running the suite instead of each example's temp project. + config.before(:suite) do + `git rev-parse --local-env-vars`.split.each { |key| ENV.delete(key) } + end + config.before do |c| unless c.metadata[:do_not_bust_cache] CodeOwnership.bust_caches!