From 7ca358bcd830b566b3137ccf482226694f757e20 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Fri, 6 Mar 2026 13:07:33 -0800 Subject: [PATCH 1/5] 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) --- src/runner.rs | 7 ++--- tests/validate_files_test.rs | 53 ++++-------------------------------- 2 files changed, 9 insertions(+), 51 deletions(-) diff --git a/src/runner.rs b/src/runner.rs index 433dea2..6f6c688 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -1,4 +1,4 @@ -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::process::Command; use error_stack::{Result, ResultExt}; @@ -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(); @@ -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)), diff --git a/tests/validate_files_test.rs b/tests/validate_files_test.rs index ca3bce9..541b5bf 100644 --- a/tests/validate_files_test.rs +++ b/tests/validate_files_test.rs @@ -145,37 +145,11 @@ fn test_validate_only_checks_codeowners_file() -> Result<(), Box> { #[test] fn test_validate_files_respects_owned_globs_with_excluded_extensions() -> Result<(), Box> { - // ============================================================================ - // 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"); @@ -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) From d2679fa256fec5223bdd6b8a3002bc67c2727afe Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Fri, 6 Mar 2026 13:10:53 -0800 Subject: [PATCH 2/5] 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. --- .github/workflows/ci.yml | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e50da77..6f2aabc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -18,15 +18,6 @@ env: CARGO_TERM_COLOR: always jobs: - check: - name: Check - runs-on: ubuntu-latest - steps: - - name: Checkout sources - uses: actions/checkout@v4 - - - name: Run cargo check - run: cargo check test: name: Test Suite runs-on: ubuntu-latest @@ -58,7 +49,6 @@ jobs: needs: - test - lints - - check outputs: new_version: ${{ steps.check_for_version_changes.outputs.new_version }} changed: ${{ steps.check_for_version_changes.outputs.changed }} From 2050667fd5bd955e2a5fa77f6afb9e424893bb09 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Fri, 6 Mar 2026 13:12:39 -0800 Subject: [PATCH 3/5] 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. --- .github/workflows/ci.yml | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6f2aabc..e442821 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -18,6 +18,20 @@ env: CARGO_TERM_COLOR: always jobs: + check: + name: Check + runs-on: ubuntu-latest + steps: + - name: Checkout sources + uses: actions/checkout@v4 + + - name: Cache Rust build artifacts + uses: Swatinem/rust-cache@v2 + with: + cache-on-failure: false + + - name: Run cargo check + run: cargo check test: name: Test Suite runs-on: ubuntu-latest @@ -25,6 +39,11 @@ jobs: - name: Checkout sources uses: actions/checkout@v4 + - name: Cache Rust build artifacts + uses: Swatinem/rust-cache@v2 + with: + cache-on-failure: false + - name: Run cargo test with backtrace run: cargo test -- --nocapture env: @@ -38,6 +57,11 @@ jobs: - name: Checkout sources uses: actions/checkout@v4 + - name: Cache Rust build artifacts + uses: Swatinem/rust-cache@v2 + with: + cache-on-failure: false + - name: Run cargo fmt run: cargo fmt --all -- --check @@ -47,6 +71,7 @@ jobs: release: runs-on: macos-latest needs: + - check - test - lints outputs: From 693e8ee50b807cb2fa0ffa6c303a489f388ed36d Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Fri, 6 Mar 2026 13:16:05 -0800 Subject: [PATCH 4/5] Revert "Add Rust build cache to CI and restore cargo check job" This reverts commit 2050667fd5bd955e2a5fa77f6afb9e424893bb09. --- .github/workflows/ci.yml | 25 ------------------------- 1 file changed, 25 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e442821..6f2aabc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -18,20 +18,6 @@ env: CARGO_TERM_COLOR: always jobs: - check: - name: Check - runs-on: ubuntu-latest - steps: - - name: Checkout sources - uses: actions/checkout@v4 - - - name: Cache Rust build artifacts - uses: Swatinem/rust-cache@v2 - with: - cache-on-failure: false - - - name: Run cargo check - run: cargo check test: name: Test Suite runs-on: ubuntu-latest @@ -39,11 +25,6 @@ jobs: - name: Checkout sources uses: actions/checkout@v4 - - name: Cache Rust build artifacts - uses: Swatinem/rust-cache@v2 - with: - cache-on-failure: false - - name: Run cargo test with backtrace run: cargo test -- --nocapture env: @@ -57,11 +38,6 @@ jobs: - name: Checkout sources uses: actions/checkout@v4 - - name: Cache Rust build artifacts - uses: Swatinem/rust-cache@v2 - with: - cache-on-failure: false - - name: Run cargo fmt run: cargo fmt --all -- --check @@ -71,7 +47,6 @@ jobs: release: runs-on: macos-latest needs: - - check - test - lints outputs: From a913cf4bb19635e67fbfd2c5c2ea2a349af9bed4 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Fri, 6 Mar 2026 13:16:18 -0800 Subject: [PATCH 5/5] Revert "Remove redundant cargo check job from CI" This reverts commit d2679fa256fec5223bdd6b8a3002bc67c2727afe. --- .github/workflows/ci.yml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6f2aabc..e50da77 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -18,6 +18,15 @@ env: CARGO_TERM_COLOR: always jobs: + check: + name: Check + runs-on: ubuntu-latest + steps: + - name: Checkout sources + uses: actions/checkout@v4 + + - name: Run cargo check + run: cargo check test: name: Test Suite runs-on: ubuntu-latest @@ -49,6 +58,7 @@ jobs: needs: - test - lints + - check outputs: new_version: ${{ steps.check_for_version_changes.outputs.new_version }} changed: ${{ steps.check_for_version_changes.outputs.changed }}