Skip to content

Fix missing Path import lost during merge of #89 #100

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dduugg merged 5 commits into from
Mar 6, 2026
Merged

Fix missing Path import lost during merge of #89 #100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7ca358b
Clean up stale comments and fix missing Path import from #89
dduugg Mar 6, 2026
d2679fa
Remove redundant cargo check job from CI
dduugg Mar 6, 2026
2050667
Add Rust build cache to CI and restore cargo check job
dduugg Mar 6, 2026
693e8ee
Revert "Add Rust build cache to CI and restore cargo check job"
dduugg Mar 6, 2026
a913cf4
Revert "Remove redundant cargo check job from CI"
dduugg Mar 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/runner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::path::PathBuf;
use std::path::{Path, PathBuf};
use std::process::Command;

use error_stack::{Result, ResultExt};
Expand Down Expand Up @@ -151,7 +151,7 @@ impl Runner {
path
};

// Apply the same filtering logic as project_builder.rs:172
// Mirror the filtering applied by ProjectBuilder when walking the project
matches_globs(relative_path, &self.config.owned_globs) && !matches_globs(relative_path, &self.config.unowned_globs)
})
.collect();
Expand Down Expand Up @@ -432,8 +432,7 @@ impl RunResult {
}
}

/// Helper function to check if a path matches any of the provided glob patterns.
/// This is used to filter files by owned_globs and unowned_globs configuration.
/// Returns true if `path` matches any of the provided glob patterns.
fn matches_globs(path: &Path, globs: &[String]) -> bool {
match path.to_str() {
Some(s) => globs.iter().any(|glob| glob_match(glob, s)),
Expand Down
53 changes: 6 additions & 47 deletions tests/validate_files_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,37 +145,11 @@ fn test_validate_only_checks_codeowners_file() -> Result<(), Box<dyn Error>> {

#[test]
fn test_validate_files_respects_owned_globs_with_excluded_extensions() -> Result<(), Box<dyn Error>> {
// ============================================================================
// THIS TEST CURRENTLY FAILS ON MAIN - IT DEMONSTRATES THE BUG
// ============================================================================
// Validates that files not matching owned_globs are silently skipped when
// validate is called with an explicit file list.
//
// BUG DESCRIPTION:
// When validate is called with a file list, it validates ALL provided files
// without checking if they match owned_globs configuration.
//
// CONFIGURATION:
// valid_project has: owned_globs = "**/*.{rb,tsx,erb}"
// Notice: .rbi files (Sorbet interface files) are NOT in this pattern
//
// EXPECTED BEHAVIOR:
// - .rbi files should be SILENTLY SKIPPED (don't match owned_globs)
// - Only .rb files should be validated against CODEOWNERS
// - Command should SUCCEED because all validated files are owned
//
// ACTUAL BEHAVIOR (BUG):
// - ALL files are validated (including .rbi files)
// - .rbi files are not in CODEOWNERS (correctly excluded during generate)
// - .rbi files are reported as "Unowned"
// - Command FAILS with validation errors
//
// ROOT CAUSE:
// src/runner.rs lines 112-143: validate_files() iterates all file_paths
// without applying the owned_globs/unowned_globs filter that
// project_builder.rs:172 uses when no files are specified
//
// FIX NEEDED:
// Filter file_paths by owned_globs and unowned_globs before validation
// ============================================================================
// valid_project owned_globs: "{gems,config,javascript,ruby,components}/**/*.{rb,tsx,erb}"
// .rbi files (Sorbet interface files) do NOT match this pattern and should be filtered.

// Setup: Create a temporary copy of valid_project fixture
let fixture_root = std::path::Path::new("tests/fixtures/valid_project");
Expand Down Expand Up @@ -219,23 +193,8 @@ fn test_validate_files_respects_owned_globs_with_excluded_extensions() -> Result
"CODEOWNERS should NOT contain .rbi files (they don't match owned_globs)"
);

// Step 2: Run validate with BOTH .rb and .rbi files in the list
// EXPECTED: .rbi files are silently skipped, only .rb files validated, succeeds
// ACTUAL (BUG): All files validated, .rbi reported as unowned, command fails
//
// ============================================================================
// THIS ASSERTION WILL FAIL ON MAIN (proving the bug exists)
// ============================================================================
//
// The command should succeed because:
// 1. .rbi files should be filtered out (don't match owned_globs)
// 2. Only .rb files should be validated
// 3. All .rb files are properly owned in CODEOWNERS
//
// But it currently fails because:
// 1. ALL files (including .rbi) are validated
// 2. .rbi files are not in CODEOWNERS
// 3. Validation error: "Unowned files detected: ruby/app/models/bank_account.rbi ..."
// Step 2: Run validate with BOTH .rb and .rbi files in the list.
// .rbi files should be silently filtered; only .rb files validated; command succeeds.
Command::cargo_bin("codeowners")?
.arg("--project-root")
.arg(project_root)
Expand Down