Skip to content

Commit 00c6788

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 00c6788

File tree

2 files changed

+304
-1
lines changed

2 files changed

+304
-1
lines changed

src/runner.rs

Lines changed: 29 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,25 @@ 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).unwrap_or(path)
126+
} else {
127+
path
128+
};
129+
130+
// Apply the same filtering logic as project_builder.rs:172
131+
matches_globs(relative_path, &self.config.owned_globs) && !matches_globs(relative_path, &self.config.unowned_globs)
132+
})
133+
.collect();
134+
135+
for file_path in filtered_paths {
117136
match team_for_file_from_codeowners(&self.run_config, &file_path) {
118137
Ok(Some(_)) => {}
119138
Ok(None) => unowned_files.push(file_path),
@@ -389,6 +408,15 @@ impl RunResult {
389408
}
390409
}
391410

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

tests/validate_files_test.rs

Lines changed: 275 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,3 +142,278 @@ 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(
191+
&bank_account_rbi,
192+
"# typed: strict\n# RBI file for BankAccount\nclass BankAccount; end\n",
193+
)?;
194+
std::fs::write(&payroll_rbi, "# typed: strict\n# RBI file for Payroll\nclass Payroll; end\n")?;
195+
196+
git_add_all_files(project_root);
197+
198+
// Step 1: Generate CODEOWNERS
199+
// This should ONLY include .rb files (not .rbi) because .rbi doesn't match owned_globs
200+
let codeowners_path = project_root.join("tmp/CODEOWNERS");
201+
Command::cargo_bin("codeowners")?
202+
.arg("--project-root")
203+
.arg(project_root)
204+
.arg("--codeowners-file-path")
205+
.arg(&codeowners_path)
206+
.arg("--no-cache")
207+
.arg("generate")
208+
.assert()
209+
.success();
210+
211+
// Verify: CODEOWNERS contains .rb files but NOT .rbi files
212+
let codeowners_content = std::fs::read_to_string(&codeowners_path)?;
213+
assert!(
214+
codeowners_content.contains("bank_account.rb"),
215+
"CODEOWNERS should contain .rb files (they match owned_globs)"
216+
);
217+
assert!(
218+
!codeowners_content.contains("bank_account.rbi"),
219+
"CODEOWNERS should NOT contain .rbi files (they don't match owned_globs)"
220+
);
221+
222+
// Step 2: Run validate with BOTH .rb and .rbi files in the list
223+
// EXPECTED: .rbi files are silently skipped, only .rb files validated, succeeds
224+
// ACTUAL (BUG): All files validated, .rbi reported as unowned, command fails
225+
//
226+
// ============================================================================
227+
// THIS ASSERTION WILL FAIL ON MAIN (proving the bug exists)
228+
// ============================================================================
229+
//
230+
// The command should succeed because:
231+
// 1. .rbi files should be filtered out (don't match owned_globs)
232+
// 2. Only .rb files should be validated
233+
// 3. All .rb files are properly owned in CODEOWNERS
234+
//
235+
// But it currently fails because:
236+
// 1. ALL files (including .rbi) are validated
237+
// 2. .rbi files are not in CODEOWNERS
238+
// 3. Validation error: "Unowned files detected: ruby/app/models/bank_account.rbi ..."
239+
Command::cargo_bin("codeowners")?
240+
.arg("--project-root")
241+
.arg(project_root)
242+
.arg("--codeowners-file-path")
243+
.arg(&codeowners_path)
244+
.arg("--no-cache")
245+
.arg("validate")
246+
// Mix .rb and .rbi files in the argument list
247+
.arg("ruby/app/models/bank_account.rb") // Should be validated (matches owned_globs)
248+
.arg("ruby/app/models/bank_account.rbi") // Should be SKIPPED (doesn't match)
249+
.arg("ruby/app/models/payroll.rb") // Should be validated (matches owned_globs)
250+
.arg("ruby/app/models/payroll.rbi") // Should be SKIPPED (doesn't match)
251+
.assert()
252+
.success()
253+
.stdout(predicate::eq(""));
254+
255+
Ok(())
256+
}
257+
258+
// ============================================================================
259+
// GLOB FILTERING TESTS: Verify validate with files respects owned_globs
260+
// ============================================================================
261+
//
262+
// These tests ensure that when validate is called with explicit file paths,
263+
// it correctly filters files based on owned_globs configuration. Files that
264+
// don't match owned_globs should be silently skipped, not reported as unowned.
265+
266+
#[test]
267+
fn test_validate_filters_multiple_non_matching_extensions() -> Result<(), Box<dyn Error>> {
268+
// Test that various file types not in owned_globs are filtered out
269+
// valid_project owned_globs: "{gems,config,javascript,ruby,components}/**/*.{rb,tsx,erb}"
270+
let fixture_root = std::path::Path::new("tests/fixtures/valid_project");
271+
let temp_dir = setup_fixture_repo(fixture_root);
272+
let project_root = temp_dir.path();
273+
274+
// Create files with extensions NOT in owned_globs
275+
std::fs::write(project_root.join("ruby/app/models/test.rbi"), "# Sorbet RBI file")?;
276+
std::fs::write(project_root.join("ruby/app/models/test.md"), "# Markdown doc")?;
277+
std::fs::write(project_root.join("ruby/app/models/test.txt"), "Plain text")?;
278+
std::fs::write(project_root.join("ruby/app/models/test.json"), "{}")?;
279+
280+
git_add_all_files(project_root);
281+
282+
let codeowners_path = project_root.join("tmp/CODEOWNERS");
283+
284+
// Generate CODEOWNERS (will only include .rb, .tsx, .erb files)
285+
Command::cargo_bin("codeowners")?
286+
.arg("--project-root")
287+
.arg(project_root)
288+
.arg("--codeowners-file-path")
289+
.arg(&codeowners_path)
290+
.arg("--no-cache")
291+
.arg("generate")
292+
.assert()
293+
.success();
294+
295+
// Validate with a mix of matching and non-matching files
296+
// All non-matching should be filtered, matching ones should succeed
297+
Command::cargo_bin("codeowners")?
298+
.arg("--project-root")
299+
.arg(project_root)
300+
.arg("--codeowners-file-path")
301+
.arg(&codeowners_path)
302+
.arg("--no-cache")
303+
.arg("validate")
304+
.arg("ruby/app/models/payroll.rb") // matches owned_globs, is owned
305+
.arg("ruby/app/models/test.rbi") // doesn't match owned_globs
306+
.arg("ruby/app/models/test.md") // doesn't match owned_globs
307+
.arg("ruby/app/models/test.txt") // doesn't match owned_globs
308+
.arg("ruby/app/models/test.json") // doesn't match owned_globs
309+
.assert()
310+
.success()
311+
.stdout(predicate::eq(""));
312+
313+
Ok(())
314+
}
315+
316+
#[test]
317+
fn test_validate_filters_files_outside_owned_directories() -> Result<(), Box<dyn Error>> {
318+
// Test that files in directories not matching owned_globs are filtered
319+
// valid_project owned_globs: "{gems,config,javascript,ruby,components}/**/*.{rb,tsx,erb}"
320+
let fixture_root = std::path::Path::new("tests/fixtures/valid_project");
321+
let temp_dir = setup_fixture_repo(fixture_root);
322+
let project_root = temp_dir.path();
323+
324+
// Create .rb files OUTSIDE the owned directories
325+
std::fs::create_dir_all(project_root.join("scripts"))?;
326+
std::fs::write(project_root.join("scripts/deploy.rb"), "# Deploy script")?;
327+
std::fs::create_dir_all(project_root.join("bin"))?;
328+
std::fs::write(project_root.join("bin/run.rb"), "# Run script")?;
329+
330+
git_add_all_files(project_root);
331+
332+
let codeowners_path = project_root.join("tmp/CODEOWNERS");
333+
334+
// Generate CODEOWNERS
335+
Command::cargo_bin("codeowners")?
336+
.arg("--project-root")
337+
.arg(project_root)
338+
.arg("--codeowners-file-path")
339+
.arg(&codeowners_path)
340+
.arg("--no-cache")
341+
.arg("generate")
342+
.assert()
343+
.success();
344+
345+
// Validate with files both inside and outside owned directories
346+
Command::cargo_bin("codeowners")?
347+
.arg("--project-root")
348+
.arg(project_root)
349+
.arg("--codeowners-file-path")
350+
.arg(&codeowners_path)
351+
.arg("--no-cache")
352+
.arg("validate")
353+
.arg("ruby/app/models/payroll.rb") // inside ruby/, matches owned_globs
354+
.arg("scripts/deploy.rb") // outside owned dirs, filtered
355+
.arg("bin/run.rb") // outside owned dirs, filtered
356+
.assert()
357+
.success()
358+
.stdout(predicate::eq(""));
359+
360+
Ok(())
361+
}
362+
363+
#[test]
364+
fn test_validate_respects_unowned_globs() -> Result<(), Box<dyn Error>> {
365+
// Test that files matching unowned_globs are filtered out even if they match owned_globs
366+
let fixture_root = std::path::Path::new("tests/fixtures/valid_project");
367+
let temp_dir = setup_fixture_repo(fixture_root);
368+
let project_root = temp_dir.path();
369+
370+
// Read and modify the config to add unowned_globs
371+
let config_path = project_root.join("config/code_ownership.yml");
372+
let config_content = std::fs::read_to_string(&config_path)?;
373+
let updated_config = config_content.replace("unowned_globs:", "unowned_globs:\n - ruby/app/models/ignored_*.rb");
374+
std::fs::write(&config_path, updated_config)?;
375+
376+
// Create a file that matches owned_globs but also matches unowned_globs
377+
std::fs::write(
378+
project_root.join("ruby/app/models/ignored_test.rb"),
379+
"# This file should be ignored via unowned_globs",
380+
)?;
381+
382+
git_add_all_files(project_root);
383+
384+
let codeowners_path = project_root.join("tmp/CODEOWNERS");
385+
386+
// Generate CODEOWNERS (ignored_test.rb should NOT be included)
387+
Command::cargo_bin("codeowners")?
388+
.arg("--project-root")
389+
.arg(project_root)
390+
.arg("--codeowners-file-path")
391+
.arg(&codeowners_path)
392+
.arg("--no-cache")
393+
.arg("generate")
394+
.assert()
395+
.success();
396+
397+
// Verify the ignored file is NOT in CODEOWNERS
398+
let codeowners_content = std::fs::read_to_string(&codeowners_path)?;
399+
assert!(
400+
!codeowners_content.contains("ignored_test.rb"),
401+
"ignored_test.rb should not be in CODEOWNERS"
402+
);
403+
404+
// Validate with the ignored file - should be filtered by unowned_globs
405+
Command::cargo_bin("codeowners")?
406+
.arg("--project-root")
407+
.arg(project_root)
408+
.arg("--codeowners-file-path")
409+
.arg(&codeowners_path)
410+
.arg("--no-cache")
411+
.arg("validate")
412+
.arg("ruby/app/models/payroll.rb") // owned, should validate
413+
.arg("ruby/app/models/ignored_test.rb") // matches unowned_globs, should be filtered
414+
.assert()
415+
.success()
416+
.stdout(predicate::eq(""));
417+
418+
Ok(())
419+
}

0 commit comments

Comments
 (0)