Honor metadata.owner in for-file for Ruby packages

src/ownership/file_owner_resolver.rs

@@ -185,16 +185,24 @@ 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(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((
+                                    team.name.clone(),
+                                    Source::Package(package_path.to_string_lossy().to_string(), package_glob),
+                                ));
+                            }
+                        }
+                        // validate rejects this package, so don't fall through to an enclosing package's owner.
+                        Err(e) => {
+                            eprintln!("Error reading ruby package: {e:?}, path: {}", pkg_yml.display());
+                            return None;
+                        }
+                    }
                 }
             }
             if glob_list_matches(rel_str, &config.javascript_package_paths) {
@@ -225,12 +233,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<String, String> {
-    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())
-}
-
 fn read_js_package_owner(path: &Path) -> std::result::Result<String, String> {
     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())?;
@@ -354,6 +356,95 @@ 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<String, Team> = 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_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<String, Team> = 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<String, Team> = 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<String, Team> = 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();

src/project_builder.rs

@@ -336,7 +336,7 @@ fn matches_globs(path: &Path, globs: &[String]) -> bool {
     }
 }
 
-fn ruby_package_owner(path: &Path) -> Result<Option<String>, Report<Error>> {
+pub(crate) fn ruby_package_owner(path: &Path) -> Result<Option<String>, Report<Error>> {
     let file = File::open(path).change_context(Error::Io)?;
     let deserializer: deserializers::RubyPackage = serde_yaml::from_reader(file).change_context(Error::SerdeYaml)?;