From b27bc5bf0851eb049e418d08ed4b318b62064346 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 24 Sep 2026 22:16:10 -0700 Subject: [PATCH 1/3] Honor metadata.owner in for-file for Ruby packages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Ruby `package.yml` that declares its owner as `metadata.owner` was ignored by `for-file`, so every file in the package came back Unowned: $ codeowners for-file packs/foo/app/models/bar.rb # metadata: { owner: Ops } Team: Unowned #85 restored `metadata.owner` for Ruby packages, but only in `project_builder::ruby_package_owner`, which drives CODEOWNERS generation and validation. `for-file` goes through a separate fast path in `file_owner_resolver`, whose own `read_ruby_package_owner` parsed the same file independently and still read only the top-level `owner` key. So the two paths disagreed about the same package: generation wrote `/packs/foo/**/** @org/ops` while `for-file` said Unowned. Delegate the fast path to `ruby_package_owner` instead of fixing the copy. Two parsers for one file is what let them drift, and a second corrected copy would leave the same opening. The fast path now inherits the function's existing tests from #85 — top-level, `metadata.owner`, both-equal, both-conflicting, and none — rather than needing a parallel set. One behavior change follows from sharing the conflict check. A `package.yml` with different `owner` and `metadata.owner` values used to resolve to the top-level value in `for-file`; it now resolves to no package owner. Validation already rejects that file ("conflicting owners ... Please use only one"), so this only affects repos that fail `validate`, and it makes `for-file` agree with it instead of silently picking one. Adds a test for `nearest_package_owner` with a `metadata.owner` package, which fails on the previous code with `called Option::unwrap() on a None value`. Related to rubyatscale/code_ownership#141 --- src/ownership/file_owner_resolver.rs | 32 +++++++++++++++++++++++++--- src/project_builder.rs | 2 +- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/src/ownership/file_owner_resolver.rs b/src/ownership/file_owner_resolver.rs index 71e5247..608d059 100644 --- a/src/ownership/file_owner_resolver.rs +++ b/src/ownership/file_owner_resolver.rs @@ -226,9 +226,9 @@ fn glob_list_matches(path: &str, globs: &[String]) -> bool { } fn read_ruby_package_owner(path: &Path) -> std::result::Result { - let file = std::fs::File::open(path).map_err(|e| e.to_string())?; - let deserializer: crate::project::deserializers::RubyPackage = serde_yaml::from_reader(file).map_err(|e| e.to_string())?; - deserializer.owner.ok_or_else(|| "Missing owner".to_string()) + crate::project_builder::ruby_package_owner(path) + .map_err(|e| e.to_string())? + .ok_or_else(|| "Missing owner".to_string()) } fn read_js_package_owner(path: &Path) -> std::result::Result { @@ -354,6 +354,32 @@ mod tests { assert_eq!(result.0, "DeepTeam"); } + #[test] + fn test_nearest_package_owner_ruby_metadata_owner() { + let td = tempdir().unwrap(); + let project_root = td.path(); + let config = build_config_for_temp("frontend/**/*", "packs/**/*", "vendored"); + + let ruby_pkg = project_root.join("packs/payroll"); + std::fs::create_dir_all(&ruby_pkg).unwrap(); + std::fs::write(ruby_pkg.join("package.yml"), "---\nmetadata:\n owner: Payroll\n").unwrap(); + + let mut tbn: HashMap = HashMap::new(); + let t = team_named("Payroll"); + tbn.insert(t.name.clone(), t); + + let rel_ruby = Path::new("packs/payroll/app/models/thing.rb"); + let ruby_owner = nearest_package_owner(project_root, rel_ruby, &config, &tbn).unwrap(); + assert_eq!(ruby_owner.0, "Payroll"); + match ruby_owner.1 { + Source::Package(pkg_path, glob) => { + assert!(pkg_path.ends_with("packs/payroll/package.yml")); + assert_eq!(glob, "packs/payroll/**/**"); + } + _ => panic!("expected Package source for ruby"), + } + } + #[test] fn test_nearest_package_owner_ruby_and_js() { let td = tempdir().unwrap(); diff --git a/src/project_builder.rs b/src/project_builder.rs index 193791b..e0d06ee 100644 --- a/src/project_builder.rs +++ b/src/project_builder.rs @@ -336,7 +336,7 @@ fn matches_globs(path: &Path, globs: &[String]) -> bool { } } -fn ruby_package_owner(path: &Path) -> Result, Report> { +pub(crate) fn ruby_package_owner(path: &Path) -> Result, Report> { let file = File::open(path).change_context(Error::Io)?; let deserializer: deserializers::RubyPackage = serde_yaml::from_reader(file).change_context(Error::SerdeYaml)?; From e5870470f7ba6ea7d72abae9fd0f205a2a842c58 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Fri, 25 Sep 2026 10:12:39 -0700 Subject: [PATCH 2/3] Stop the for-file package walk on an unreadable package.yml Review on #131 found that delegating to `ruby_package_owner` collapsed two different outcomes into one. `read_ruby_package_owner` mapped both "package has no owner" (`Ok(None)`) and "package is invalid" (`Err`) to the same error string, and `nearest_package_owner` kept walking up on either. For a nested pack whose inner `package.yml` has conflicting owners, that meant `for-file` skipped the inner package and reported the outer package's team: packs/outer/package.yml owner: Outer packs/outer/inner/package.yml owner: InnerA / metadata.owner: InnerB $ codeowners for-file packs/outer/inner/x.rb Team: Outer # confident, and wrong `generate` and `validate` hard-error on that same package, so `for-file` was inventing an answer where the rest of the tool refuses to give one. Match the three outcomes the generate path already distinguishes: - `Ok(Some(owner))` the package owns the file - `Ok(None)` no owner declared; keep walking, so an enclosing package still applies, as it does in generate - `Err` stop and report no package owner The same rule now covers a malformed `package.yml`, which previously also fell through to the enclosing package. `validate` rejects that too. Removing `read_ruby_package_owner` also drops the `.map_err(|e| e.to_string())` that reduced a conflict error to "IO operation failed" and lost the path and conflict detail. Nothing surfaced that string, but it no longer exists to lose. Adds three `nearest_package_owner` tests. The nested-conflict case fails on the previous commit; the single-package conflict and ownerless-inner-package cases passed already and pin behavior the earlier commit relied on without testing. --- src/ownership/file_owner_resolver.rs | 95 +++++++++++++++++++++++----- 1 file changed, 79 insertions(+), 16 deletions(-) diff --git a/src/ownership/file_owner_resolver.rs b/src/ownership/file_owner_resolver.rs index 608d059..8d1eb7e 100644 --- a/src/ownership/file_owner_resolver.rs +++ b/src/ownership/file_owner_resolver.rs @@ -185,16 +185,22 @@ fn nearest_package_owner( if let Some(rel_str) = parent_rel.to_str() { if glob_list_matches(rel_str, &config.ruby_package_paths) { let pkg_yml = current.join("package.yml"); - if pkg_yml.exists() - && let Ok(owner) = read_ruby_package_owner(&pkg_yml) - && let Some(team) = teams_by_name.get(&owner) - { - let package_path = parent_rel.join("package.yml"); - let package_glob = format!("{rel_str}/**/**"); - return Some(( - team.name.clone(), - Source::Package(package_path.to_string_lossy().to_string(), package_glob), - )); + if pkg_yml.exists() { + match crate::project_builder::ruby_package_owner(&pkg_yml) { + Ok(Some(owner)) => { + if let Some(team) = teams_by_name.get(&owner) { + let package_path = parent_rel.join("package.yml"); + let package_glob = format!("{rel_str}/**/**"); + return Some(( + team.name.clone(), + Source::Package(package_path.to_string_lossy().to_string(), package_glob), + )); + } + } + Ok(None) => {} + // validate rejects this package, so don't fall through to an enclosing package's owner. + Err(_) => return None, + } } } if glob_list_matches(rel_str, &config.javascript_package_paths) { @@ -225,12 +231,6 @@ fn glob_list_matches(path: &str, globs: &[String]) -> bool { globs.iter().any(|g| glob_match(g, path)) } -fn read_ruby_package_owner(path: &Path) -> std::result::Result { - crate::project_builder::ruby_package_owner(path) - .map_err(|e| e.to_string())? - .ok_or_else(|| "Missing owner".to_string()) -} - fn read_js_package_owner(path: &Path) -> std::result::Result { let file = std::fs::File::open(path).map_err(|e| e.to_string())?; let deserializer: crate::project::deserializers::JavascriptPackage = serde_json::from_reader(file).map_err(|e| e.to_string())?; @@ -380,6 +380,69 @@ mod tests { } } + #[test] + fn test_nearest_package_owner_ruby_conflicting_owners_yields_none() { + let td = tempdir().unwrap(); + let project_root = td.path(); + let config = build_config_for_temp("frontend/**/*", "packs/**/*", "vendored"); + + let ruby_pkg = project_root.join("packs/payroll"); + std::fs::create_dir_all(&ruby_pkg).unwrap(); + std::fs::write(ruby_pkg.join("package.yml"), "---\nowner: Payroll\nmetadata:\n owner: Benefits\n").unwrap(); + + let mut tbn: HashMap = HashMap::new(); + for name in ["Payroll", "Benefits"] { + let t = team_named(name); + tbn.insert(t.name.clone(), t); + } + + let rel_ruby = Path::new("packs/payroll/app/models/thing.rb"); + assert!(nearest_package_owner(project_root, rel_ruby, &config, &tbn).is_none()); + } + + #[test] + fn test_nearest_package_owner_ruby_conflict_does_not_fall_through_to_outer_package() { + let td = tempdir().unwrap(); + let project_root = td.path(); + let config = build_config_for_temp("frontend/**/*", "packs/**/*", "vendored"); + + let outer_pkg = project_root.join("packs/outer"); + let inner_pkg = outer_pkg.join("inner"); + std::fs::create_dir_all(&inner_pkg).unwrap(); + std::fs::write(outer_pkg.join("package.yml"), "---\nowner: Outer\n").unwrap(); + std::fs::write(inner_pkg.join("package.yml"), "---\nowner: InnerA\nmetadata:\n owner: InnerB\n").unwrap(); + + let mut tbn: HashMap = HashMap::new(); + for name in ["Outer", "InnerA", "InnerB"] { + let t = team_named(name); + tbn.insert(t.name.clone(), t); + } + + let rel_ruby = Path::new("packs/outer/inner/x.rb"); + assert!(nearest_package_owner(project_root, rel_ruby, &config, &tbn).is_none()); + } + + #[test] + fn test_nearest_package_owner_ruby_ownerless_inner_package_falls_through_to_outer() { + let td = tempdir().unwrap(); + let project_root = td.path(); + let config = build_config_for_temp("frontend/**/*", "packs/**/*", "vendored"); + + let outer_pkg = project_root.join("packs/outer"); + let inner_pkg = outer_pkg.join("inner"); + std::fs::create_dir_all(&inner_pkg).unwrap(); + std::fs::write(outer_pkg.join("package.yml"), "---\nowner: Outer\n").unwrap(); + std::fs::write(inner_pkg.join("package.yml"), "---\nenforce_dependencies: true\n").unwrap(); + + let mut tbn: HashMap = HashMap::new(); + let t = team_named("Outer"); + tbn.insert(t.name.clone(), t); + + let rel_ruby = Path::new("packs/outer/inner/x.rb"); + let owner = nearest_package_owner(project_root, rel_ruby, &config, &tbn).unwrap(); + assert_eq!(owner.0, "Outer"); + } + #[test] fn test_nearest_package_owner_ruby_and_js() { let td = tempdir().unwrap(); From 9813c8ee7d9129ee2f86e17118d528b8c26b5dd1 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Fri, 25 Sep 2026 10:18:52 -0700 Subject: [PATCH 3/3] Report an unreadable package.yml from for-file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit made `for-file` stop at a package whose `package.yml` cannot be read, but it discarded the error, so a conflict or YAML error gave no hint why the file came back Unowned. Print it to stderr with `{e:?}`, following the existing `load_teams` pattern in this file, which keeps the full report — including the "conflicting owners ... Please use only one" detail that `e.to_string()` would reduce to "IO operation failed". It goes to stderr, so `--json` output on stdout stays parseable. Also collapse the `Ok(Some)` / `Ok(None)` arms into one with `Option::and_then`, which removes a level of nesting without changing behavior: an ownerless package still falls through to an enclosing one. --- src/ownership/file_owner_resolver.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/ownership/file_owner_resolver.rs b/src/ownership/file_owner_resolver.rs index 8d1eb7e..b0a23dd 100644 --- a/src/ownership/file_owner_resolver.rs +++ b/src/ownership/file_owner_resolver.rs @@ -187,8 +187,8 @@ fn nearest_package_owner( let pkg_yml = current.join("package.yml"); if pkg_yml.exists() { match crate::project_builder::ruby_package_owner(&pkg_yml) { - Ok(Some(owner)) => { - if let Some(team) = teams_by_name.get(&owner) { + Ok(owner) => { + if let Some(team) = owner.and_then(|owner| teams_by_name.get(&owner)) { let package_path = parent_rel.join("package.yml"); let package_glob = format!("{rel_str}/**/**"); return Some(( @@ -197,9 +197,11 @@ fn nearest_package_owner( )); } } - Ok(None) => {} // validate rejects this package, so don't fall through to an enclosing package's owner. - Err(_) => return None, + Err(e) => { + eprintln!("Error reading ruby package: {e:?}, path: {}", pkg_yml.display()); + return None; + } } } }