From 3c78e1121647819bab18e178a7a53e2607a7d228 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Fri, 24 Jul 2026 23:43:33 -0700 Subject: [PATCH] Consider new, untracked team config files during validate/generate rubyatscale/code_ownership#149: adding a new team was silently ignored until the new team.yml was `git add`-ed. The project walk deliberately excludes untracked files (codeowners-rs#46/#74/#76 - so a developer can keep scratch files around locally without validate forcing them to assign an owner), but that exclusion also covered team config files, which define real ownership rules the moment they exist rather than being scratch work anyone would leave uncommitted on purpose. Exempt files matching `team_file_glob` from the tracked-files check specifically, leaving ordinary source files subject to it as before. Reproduced the exact bug against a real build first (a new team owning an already-tracked file, e.g. README.md, was reported unowned because the team file disappeared from the walk while README.md - already committed - did not), confirmed a fix, then found and preserved a directly conflicting existing test (test_skip_untracked_files) that relies on the untracked-scratch-file behavior on purpose, which a first, broader attempt at this fix would have silently regressed. Added two tests: one reproducing #149 end to end (new untracked team + existing tracked file -> validate succeeds), and one pinning that an untracked *source* file (non-team) is still correctly ignored, so the scoped nature of this fix doesn't regress by accident later. Verified: full `cargo test` suite (all 83+ tests, including the pre-existing test_skip_untracked_files), cargo clippy --all-targets, and cargo fmt --check all clean. --- src/project_builder.rs | 14 +++++++ tests/untracked_new_file_test.rs | 63 +++++++++++++++++++++++++++++ tests/untracked_source_file_test.rs | 40 ++++++++++++++++++ 3 files changed, 117 insertions(+) create mode 100644 tests/untracked_new_file_test.rs create mode 100644 tests/untracked_source_file_test.rs diff --git a/src/project_builder.rs b/src/project_builder.rs index e717b65..689fd10 100644 --- a/src/project_builder.rs +++ b/src/project_builder.rs @@ -73,16 +73,30 @@ impl<'a> ProjectBuilder<'a> { // Prune traversal early: skip heavy and irrelevant directories let ignore_dirs = self.config.ignore_dirs.clone(); + let team_file_glob = self.config.team_file_glob.clone(); let base_path = self.base_path.clone(); let tracked_files = tracked_files::find_tracked_files(&self.base_path); builder.filter_entry(move |entry: &DirEntry| { let path = entry.path(); let file_name = entry.file_name().to_str().unwrap_or(""); + // Team config files are exempt from the tracked-files check below: a + // freshly-created team.yml defines real ownership rules the moment it + // exists, not scratch work someone might never commit, so requiring + // `git add` before a new team is considered would silently produce + // "unowned file" errors for anything that should now belong to it + // (rubyatscale/code_ownership#149). Ordinary source files are left + // subject to the tracked-files check, preserving the original, + // intentional behavior of not forcing ownership onto untracked + // scratch files a developer has no plan to commit (#46/#74/#76). + let is_team_file = path + .strip_prefix(&base_path) + .is_ok_and(|relative_path| matches_globs(relative_path, &team_file_glob)); if let Some(tracked_files) = &tracked_files && let Some(ft) = entry.file_type() && ft.is_file() && !tracked_files.contains_key(path) + && !is_team_file { return false; } diff --git a/tests/untracked_new_file_test.rs b/tests/untracked_new_file_test.rs new file mode 100644 index 0000000..861a01b --- /dev/null +++ b/tests/untracked_new_file_test.rs @@ -0,0 +1,63 @@ +use assert_cmd::prelude::*; +use std::{error::Error, fs, path::Path, process::Command}; + +mod common; +use common::{git_add_all_files, setup_fixture_repo}; + +const FIXTURE: &str = "tests/fixtures/valid_project"; + +/// rubyatscale/code_ownership#149: adding a new team config must not require +/// `git add`-ing it first. The project walk excludes untracked files by +/// design (see tests/untracked_source_file_test.rs and codeowners-rs#46/#74/ +/// #76 - it lets a developer keep scratch files around without being forced +/// to assign them an owner), but that exclusion used to apply to team config +/// files too - so a brand-new, not-yet-staged team file was invisible to the +/// walk, and any *already-tracked* file that should newly become owned by +/// that team was reported as unowned instead (the team file disappears from +/// the walk, but the file it's supposed to own is still there and still +/// expected to have an owner - simply making both files new/untracked +/// together hides the bug, since then neither is visible and no mismatch is +/// ever observed). +/// +/// This mirrors the issue's exact repro shape: README.md already exists and +/// is already committed (as it would be from `rails new` or any prior +/// commit); only the *new team* and the *config edit adding README.md to +/// owned_globs* are freshly created and not yet staged. +#[test] +fn test_validate_succeeds_after_adding_new_untracked_team_for_an_existing_tracked_file() -> Result<(), Box> { + let temp_dir = setup_fixture_repo(Path::new(FIXTURE)); + let project_root = temp_dir.path(); + // README.md is part of the initial commit, so it's already tracked + // before the new team is ever introduced. + fs::write(project_root.join("README.md"), "# hello\n")?; + git_add_all_files(project_root); + Command::new("git") + .args(["commit", "-m", "initial"]) + .current_dir(project_root) + .output()?; + + // A brand-new team config, added later and never staged. + fs::write( + project_root.join("config/teams/docs.yml"), + "name: Docs\ngithub:\n team: '@DocsTeam'\nowned_globs:\n - README.md\n", + )?; + + let config_path = project_root.join("config/code_ownership.yml"); + let config = fs::read_to_string(&config_path)?; + let updated_config = config.replacen("owned_globs:\n", "owned_globs:\n - \"README.md\"\n", 1); + assert_ne!( + config, updated_config, + "expected to find and extend the owned_globs list in the fixture config" + ); + fs::write(&config_path, updated_config)?; + + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .arg("generate-and-validate") + .assert() + .success(); + + Ok(()) +} diff --git a/tests/untracked_source_file_test.rs b/tests/untracked_source_file_test.rs new file mode 100644 index 0000000..6d19465 --- /dev/null +++ b/tests/untracked_source_file_test.rs @@ -0,0 +1,40 @@ +use assert_cmd::prelude::*; +use std::{error::Error, fs, path::Path, process::Command}; + +mod common; +use common::{git_add_all_files, setup_fixture_repo}; + +const FIXTURE: &str = "tests/fixtures/valid_project"; + +/// The fix for rubyatscale/code_ownership#149 (see untracked_new_file_test.rs) +/// only exempts team config files from the tracked-files check - it must NOT +/// broaden inclusion to untracked source files in general. That exclusion is +/// intentional (codeowners-rs#46/#74/#76: "don't fail when untracked git +/// files aren't in codeowners"), letting a developer keep a scratch file +/// around locally without `validate` forcing them to assign it an owner. +#[test] +fn test_untracked_source_file_is_still_ignored() -> Result<(), Box> { + let temp_dir = setup_fixture_repo(Path::new(FIXTURE)); + let project_root = temp_dir.path(); + git_add_all_files(project_root); + Command::new("git") + .args(["commit", "-m", "initial"]) + .current_dir(project_root) + .output()?; + + // A new source file matching owned_globs, but never staged. If it were + // included, this would fail with "missing ownership" since nothing + // claims it; if it's correctly ignored (matching pre-#149-fix behavior + // for non-team files), validation succeeds unchanged. + fs::write(project_root.join("ruby/app/models/scratch.rb"), "class Scratch; end\n")?; + + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .arg("generate-and-validate") + .assert() + .success(); + + Ok(()) +}