Skip to content

Commit c86e800

Browse files
authored
Fix missing Path import lost during merge of #89 (#100)
* Clean up stale comments and fix missing Path import from #89 - Remove pre-fix bug description block from test_validate_files_respects_owned_globs_with_excluded_extensions; replace with a concise description of what the test verifies - Correct the simplified owned_globs pattern in the comment to match the actual fixture config - Replace "THIS ASSERTION WILL FAIL ON MAIN" prose with a one-line summary - Update matches_globs doc comment to describe the function rather than its callsites - Replace brittle project_builder.rs:172 line-number reference with a description of intent - Add missing `use std::path::Path` import (compile error introduced in #89) * Remove redundant cargo check job from CI The bare `cargo check` job only checked the lib/bin crates and was strictly less thorough than the existing Test Suite and Lints jobs, both of which compile all targets. The redundant job also introduced a stale-cache risk: if its artifacts were reused from a prior run, compile errors in source files could go undetected. Removing it so that Test Suite (cargo test) and Lints (cargo clippy --all-targets) are the authoritative compile checks. * Add Rust build cache to CI and restore cargo check job Uses Swatinem/rust-cache@v2 in the Check, Test Suite, and Lints jobs. Setting cache-on-failure: false (the default) ensures that a failed build never writes a stale cache entry, which is how a missing import in #89 was able to pass CI despite being a compile error. Restores the cargo check job that was removed in the prior commit, since it gives a clearer signal for compile errors than a mixed test/lint failure. * Revert "Add Rust build cache to CI and restore cargo check job" This reverts commit 2050667. * Revert "Remove redundant cargo check job from CI" This reverts commit d2679fa.
1 parent 52ee23c commit c86e800

File tree

2 files changed

+9
-51
lines changed

2 files changed

+9
-51
lines changed

src/runner.rs

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use std::path::PathBuf;
1+
use std::path::{Path, PathBuf};
22
use std::process::Command;
33

44
use error_stack::{Result, ResultExt};
@@ -151,7 +151,7 @@ impl Runner {
151151
path
152152
};
153153

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

435-
/// Helper function to check if a path matches any of the provided glob patterns.
436-
/// This is used to filter files by owned_globs and unowned_globs configuration.
435+
/// Returns true if `path` matches any of the provided glob patterns.
437436
fn matches_globs(path: &Path, globs: &[String]) -> bool {
438437
match path.to_str() {
439438
Some(s) => globs.iter().any(|glob| glob_match(glob, s)),

tests/validate_files_test.rs

Lines changed: 6 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -145,37 +145,11 @@ fn test_validate_only_checks_codeowners_file() -> Result<(), Box<dyn Error>> {
145145

146146
#[test]
147147
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-
// ============================================================================
148+
// Validates that files not matching owned_globs are silently skipped when
149+
// validate is called with an explicit file list.
151150
//
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-
// ============================================================================
151+
// valid_project owned_globs: "{gems,config,javascript,ruby,components}/**/*.{rb,tsx,erb}"
152+
// .rbi files (Sorbet interface files) do NOT match this pattern and should be filtered.
179153

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

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 ..."
196+
// Step 2: Run validate with BOTH .rb and .rbi files in the list.
197+
// .rbi files should be silently filtered; only .rb files validated; command succeeds.
239198
Command::cargo_bin("codeowners")?
240199
.arg("--project-root")
241200
.arg(project_root)

0 commit comments

Comments
 (0)