Skip to content

Commit 9e9aa9d

Browse files
committed
Fix validate to respect owned_globs when files specified
When validate was called with a file list, it validated all provided files without filtering by owned_globs configuration. This caused files that should be skipped (like .rbi when owned_globs only includes .rb) to be incorrectly reported as unowned. The fix filters file paths by owned_globs and unowned_globs before validation, making behavior consistent with validate without files. Added test using .rbi files to prove the bug existed and verify the fix works correctly.
1 parent c3cace6 commit 9e9aa9d

File tree

2 files changed

+140
-1
lines changed

2 files changed

+140
-1
lines changed

src/runner.rs

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
use std::{path::Path, process::Command};
22

33
use error_stack::{Result, ResultExt};
4+
use fast_glob::glob_match;
45
use serde::Serialize;
56

67
use crate::{
@@ -113,7 +114,27 @@ impl Runner {
113114
let mut unowned_files = Vec::new();
114115
let mut io_errors = Vec::new();
115116

116-
for file_path in file_paths {
117+
// Filter files based on owned_globs and unowned_globs configuration
118+
// Only validate files that match owned_globs and don't match unowned_globs
119+
let filtered_paths: Vec<String> = file_paths
120+
.into_iter()
121+
.filter(|file_path| {
122+
// Convert to relative path for glob matching
123+
let path = Path::new(file_path);
124+
let relative_path = if path.is_absolute() {
125+
path.strip_prefix(&self.run_config.project_root)
126+
.unwrap_or(path)
127+
} else {
128+
path
129+
};
130+
131+
// Apply the same filtering logic as project_builder.rs:172
132+
matches_globs(relative_path, &self.config.owned_globs)
133+
&& !matches_globs(relative_path, &self.config.unowned_globs)
134+
})
135+
.collect();
136+
137+
for file_path in filtered_paths {
117138
match team_for_file_from_codeowners(&self.run_config, &file_path) {
118139
Ok(Some(_)) => {}
119140
Ok(None) => unowned_files.push(file_path),
@@ -389,6 +410,15 @@ impl RunResult {
389410
}
390411
}
391412

413+
/// Helper function to check if a path matches any of the provided glob patterns.
414+
/// This is used to filter files by owned_globs and unowned_globs configuration.
415+
fn matches_globs(path: &Path, globs: &[String]) -> bool {
416+
match path.to_str() {
417+
Some(s) => globs.iter().any(|glob| glob_match(glob, s)),
418+
None => false,
419+
}
420+
}
421+
392422
#[cfg(test)]
393423
mod tests {
394424
use super::*;

tests/validate_files_test.rs

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,3 +142,112 @@ fn test_validate_only_checks_codeowners_file() -> Result<(), Box<dyn Error>> {
142142

143143
Ok(())
144144
}
145+
146+
#[test]
147+
fn test_validate_files_respects_owned_globs_with_excluded_extensions() -> Result<(), Box<dyn Error>> {
148+
// ============================================================================
149+
// THIS TEST CURRENTLY FAILS ON MAIN - IT DEMONSTRATES THE BUG
150+
// ============================================================================
151+
//
152+
// BUG DESCRIPTION:
153+
// When validate is called with a file list, it validates ALL provided files
154+
// without checking if they match owned_globs configuration.
155+
//
156+
// CONFIGURATION:
157+
// valid_project has: owned_globs = "**/*.{rb,tsx,erb}"
158+
// Notice: .rbi files (Sorbet interface files) are NOT in this pattern
159+
//
160+
// EXPECTED BEHAVIOR:
161+
// - .rbi files should be SILENTLY SKIPPED (don't match owned_globs)
162+
// - Only .rb files should be validated against CODEOWNERS
163+
// - Command should SUCCEED because all validated files are owned
164+
//
165+
// ACTUAL BEHAVIOR (BUG):
166+
// - ALL files are validated (including .rbi files)
167+
// - .rbi files are not in CODEOWNERS (correctly excluded during generate)
168+
// - .rbi files are reported as "Unowned"
169+
// - Command FAILS with validation errors
170+
//
171+
// ROOT CAUSE:
172+
// src/runner.rs lines 112-143: validate_files() iterates all file_paths
173+
// without applying the owned_globs/unowned_globs filter that
174+
// project_builder.rs:172 uses when no files are specified
175+
//
176+
// FIX NEEDED:
177+
// Filter file_paths by owned_globs and unowned_globs before validation
178+
// ============================================================================
179+
180+
// Setup: Create a temporary copy of valid_project fixture
181+
let fixture_root = std::path::Path::new("tests/fixtures/valid_project");
182+
let temp_dir = setup_fixture_repo(fixture_root);
183+
let project_root = temp_dir.path();
184+
185+
// Create .rbi files (Sorbet interface files) that do NOT match owned_globs
186+
// These files should be ignored by validate when specified in the file list
187+
let bank_account_rbi = project_root.join("ruby/app/models/bank_account.rbi");
188+
let payroll_rbi = project_root.join("ruby/app/models/payroll.rbi");
189+
190+
std::fs::write(&bank_account_rbi, "# typed: strict\n# RBI file for BankAccount\nclass BankAccount; end\n")?;
191+
std::fs::write(&payroll_rbi, "# typed: strict\n# RBI file for Payroll\nclass Payroll; end\n")?;
192+
193+
git_add_all_files(project_root);
194+
195+
// Step 1: Generate CODEOWNERS
196+
// This should ONLY include .rb files (not .rbi) because .rbi doesn't match owned_globs
197+
let codeowners_path = project_root.join("tmp/CODEOWNERS");
198+
Command::cargo_bin("codeowners")?
199+
.arg("--project-root")
200+
.arg(project_root)
201+
.arg("--codeowners-file-path")
202+
.arg(&codeowners_path)
203+
.arg("--no-cache")
204+
.arg("generate")
205+
.assert()
206+
.success();
207+
208+
// Verify: CODEOWNERS contains .rb files but NOT .rbi files
209+
let codeowners_content = std::fs::read_to_string(&codeowners_path)?;
210+
assert!(
211+
codeowners_content.contains("bank_account.rb"),
212+
"CODEOWNERS should contain .rb files (they match owned_globs)"
213+
);
214+
assert!(
215+
!codeowners_content.contains("bank_account.rbi"),
216+
"CODEOWNERS should NOT contain .rbi files (they don't match owned_globs)"
217+
);
218+
219+
// Step 2: Run validate with BOTH .rb and .rbi files in the list
220+
// EXPECTED: .rbi files are silently skipped, only .rb files validated, succeeds
221+
// ACTUAL (BUG): All files validated, .rbi reported as unowned, command fails
222+
//
223+
// ============================================================================
224+
// THIS ASSERTION WILL FAIL ON MAIN (proving the bug exists)
225+
// ============================================================================
226+
//
227+
// The command should succeed because:
228+
// 1. .rbi files should be filtered out (don't match owned_globs)
229+
// 2. Only .rb files should be validated
230+
// 3. All .rb files are properly owned in CODEOWNERS
231+
//
232+
// But it currently fails because:
233+
// 1. ALL files (including .rbi) are validated
234+
// 2. .rbi files are not in CODEOWNERS
235+
// 3. Validation error: "Unowned files detected: ruby/app/models/bank_account.rbi ..."
236+
Command::cargo_bin("codeowners")?
237+
.arg("--project-root")
238+
.arg(project_root)
239+
.arg("--codeowners-file-path")
240+
.arg(&codeowners_path)
241+
.arg("--no-cache")
242+
.arg("validate")
243+
// Mix .rb and .rbi files in the argument list
244+
.arg("ruby/app/models/bank_account.rb") // Should be validated (matches owned_globs)
245+
.arg("ruby/app/models/bank_account.rbi") // Should be SKIPPED (doesn't match)
246+
.arg("ruby/app/models/payroll.rb") // Should be validated (matches owned_globs)
247+
.arg("ruby/app/models/payroll.rbi") // Should be SKIPPED (doesn't match)
248+
.assert()
249+
.success()
250+
.stdout(predicate::eq(""));
251+
252+
Ok(())
253+
}

0 commit comments

Comments
 (0)