Represent tool sources with a shared enum (#1905)

* Represent tool sources with a shared enum

* Simplify tool source deserialization and construction

Author: mre

ci/Cargo.lock

@@ -13,3 +13,8 @@ workspace = true
 
 [dependencies]
 anyhow.workspace = true
+serde.workspace = true
+
+[dev-dependencies]
+serde_json.workspace = true
+serde-saphyr.workspace = true

ci/crates/github-repo/Cargo.toml

@@ -1,15 +1,25 @@
-//! Shared GitHub repository URL classification.
+//! Shared tool source and GitHub repository URL classification.
 
 use anyhow::{Context, Result, ensure};
+use serde::{Deserialize, Serialize, Serializer};
 
-/// A repository parsed from a GitHub HTTP(S) URL, borrowing its owner and name.
-#[derive(Debug, Clone, Copy, PartialEq, Eq)]
-pub struct GithubRepo<'a> {
-    owner: &'a str,
-    name: &'a str,
+/// A repository parsed from a GitHub HTTP(S) URL.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct GithubRepo {
+    owner: String,
+    name: String,
+    original_url: String,
 }
 
-impl<'a> TryFrom<&'a str> for GithubRepo<'a> {
+impl GithubRepo {
+    /// Returns the original URL without normalizing its spelling or trailing slashes.
+    #[must_use]
+    pub fn as_str(&self) -> &str {
+        &self.original_url
+    }
+}
+
+impl TryFrom<&str> for GithubRepo {
     type Error = anyhow::Error;
 
     /// Parses an HTTP(S) GitHub repository URL, ignoring trailing slashes.
@@ -18,8 +28,8 @@ impl<'a> TryFrom<&'a str> for GithubRepo<'a> {
     ///
     /// Returns an error for an unsupported prefix, missing owner or repository,
     /// or a repository path containing a subpath.
-    fn try_from(url: &'a str) -> Result<Self> {
-        let url = url.trim_end_matches('/');
+    fn try_from(original_url: &str) -> Result<Self> {
+        let url = original_url.trim_end_matches('/');
         let path = url
             .strip_prefix("https://github.com/")
             .or_else(|| url.strip_prefix("http://github.com/"))
@@ -29,44 +39,86 @@ impl<'a> TryFrom<&'a str> for GithubRepo<'a> {
             !owner.is_empty() && !name.is_empty() && !name.contains('/'),
             "Expected a repository URL with no subpath"
         );
-        Ok(Self { owner, name })
+        Ok(Self {
+            owner: owner.to_owned(),
+            name: name.to_owned(),
+            original_url: original_url.to_owned(),
+        })
     }
 }
 
-impl std::fmt::Display for GithubRepo<'_> {
+impl std::fmt::Display for GithubRepo {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         write!(f, "{}/{}", self.owner, self.name)
     }
 }
 
+/// A tool's source URL, classified without rejecting unsupported or malformed URLs.
+///
+/// Serialized as the original plain string, not as a tagged enum.
+#[derive(Debug, Clone, Deserialize, PartialEq, Eq)]
+#[serde(from = "String")]
+pub enum ToolSource {
+    /// A supported GitHub repository URL.
+    Github(GithubRepo),
+    /// Any other source string, retained verbatim for manual review.
+    Other(String),
+}
+
+impl ToolSource {
+    /// Returns the original source string without URL normalization.
+    #[must_use]
+    pub fn as_str(&self) -> &str {
+        match self {
+            Self::Github(repo) => repo.as_str(),
+            Self::Other(source) => source,
+        }
+    }
+}
+
+impl From<String> for ToolSource {
+    fn from(source: String) -> Self {
+        GithubRepo::try_from(source.as_str()).map_or(Self::Other(source), Self::Github)
+    }
+}
+
+impl From<&str> for ToolSource {
+    fn from(source: &str) -> Self {
+        GithubRepo::try_from(source).map_or_else(|_| Self::Other(source.to_owned()), Self::Github)
+    }
+}
+
+impl std::fmt::Display for ToolSource {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.write_str(self.as_str())
+    }
+}
+
+impl Serialize for ToolSource {
+    fn serialize<S: Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
+        serializer.serialize_str(self.as_str())
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
 
     #[test]
     fn parses_plain_github_url() -> Result<()> {
         let repo = GithubRepo::try_from("https://github.com/owner/repo")?;
-        assert_eq!(
-            repo,
-            GithubRepo {
-                owner: "owner",
-                name: "repo"
-            }
-        );
+        assert_eq!(repo.owner, "owner");
+        assert_eq!(repo.name, "repo");
+        assert_eq!(repo.as_str(), "https://github.com/owner/repo");
         assert_eq!(repo.to_string(), "owner/repo");
         Ok(())
     }
 
     #[test]
     fn parses_trailing_slash() -> Result<()> {
         let repo = GithubRepo::try_from("https://github.com/owner/repo/")?;
-        assert_eq!(
-            repo,
-            GithubRepo {
-                owner: "owner",
-                name: "repo"
-            }
-        );
+        assert_eq!(repo.to_string(), "owner/repo");
+        assert_eq!(repo.as_str(), "https://github.com/owner/repo/");
         Ok(())
     }
 
@@ -113,6 +165,66 @@ mod tests {
         }
     }
 
+    #[test]
+    fn source_variants_preserve_raw_strings_through_serde() -> Result<()> {
+        for (raw, github) in [
+            ("https://github.com/Owner/Repo.git", true),
+            ("http://github.com/Owner/Repo///", true),
+            ("https://github.com/owner/repo?tab=readme", true),
+            ("https://github.com/owner/repo#readme", true),
+            ("https://github.com/owner/repo%2Ftree", true),
+            ("https://github.com/owner/repo ", true),
+            ("https://gitlab.com/owner/repo/", false),
+            ("https://github.com/owner/repo/tree/main", false),
+            ("https://github.com/owner/", false),
+            ("https://github.com//repo", false),
+            ("https://GitHub.com/owner/repo", false),
+            (" https://github.com/owner/repo", false),
+            ("git@github.com:owner/repo.git", false),
+            ("not a URL", false),
+            ("", false),
+        ] {
+            let encoded = serde_json::to_string(raw)?;
+            for source in [
+                ToolSource::from(raw),
+                ToolSource::from(raw.to_owned()),
+                serde_json::from_str::<ToolSource>(&encoded)?,
+                serde_saphyr::from_str::<ToolSource>(&encoded)?,
+            ] {
+                assert_eq!(matches!(source, ToolSource::Github(_)), github, "{raw}");
+                assert_eq!(source.as_str(), raw);
+                assert_eq!(source.to_string(), raw);
+                assert_eq!(serde_json::to_string(&source)?, encoded);
+            }
+        }
+        Ok(())
+    }
+
+    #[test]
+    fn source_deserialization_matches_plain_string_acceptance() {
+        for raw in ["null", "42", "true", "[]", r#"{"Github":"owner/repo"}"#] {
+            assert!(serde_json::from_str::<ToolSource>(raw).is_err(), "{raw}");
+            assert_eq!(
+                serde_saphyr::from_str::<ToolSource>(raw).ok(),
+                serde_saphyr::from_str::<String>(raw)
+                    .ok()
+                    .map(ToolSource::from),
+                "{raw}"
+            );
+        }
+    }
+
+    #[test]
+    fn repository_owns_its_url() -> Result<()> {
+        let repo = {
+            let url = String::from("http://github.com/Owner/Repo///");
+            GithubRepo::try_from(url.as_str())?
+        };
+        assert_eq!(repo.to_string(), "Owner/Repo");
+        assert_eq!(repo.as_str(), "http://github.com/Owner/Repo///");
+        Ok(())
+    }
+
     #[test]
     fn rejects_subpath() {
         assert!(GithubRepo::try_from("https://github.com/owner/repo/tree/main/subdir").is_err());
@@ -138,8 +250,9 @@ mod tests {
             assert_eq!(
                 GithubRepo::try_from(url).ok(),
                 Some(GithubRepo {
-                    owner: "owner",
-                    name: "repo"
+                    owner: "owner".into(),
+                    name: "repo".into(),
+                    original_url: url.into(),
                 })
             );
         }

ci/crates/github-repo/src/lib.rs

@@ -1,6 +1,7 @@
 //! Catalog input paths and tool loading.
 
 use anyhow::{Context, Result, ensure};
+use github_repo::ToolSource;
 use serde::Deserialize;
 use std::fmt;
 use std::path::{Component, PathBuf};
@@ -40,7 +41,7 @@ impl fmt::Display for ToolPath {
 #[derive(Debug, Deserialize)]
 pub struct ToolEntry {
     pub name: String,
-    pub source: Option<String>,
+    pub source: Option<ToolSource>,
     pub homepage: Option<String>,
 }
 
@@ -104,6 +105,30 @@ mod tests {
         Ok(())
     }
 
+    #[test]
+    fn tool_sources_are_classified_during_deserialization() -> Result<()> {
+        let tool: ToolEntry =
+            serde_saphyr::from_str("name: Example\nsource: http://github.com/Owner/Repo///\n")?;
+        let Some(ToolSource::Github(repo)) = tool.source else {
+            anyhow::bail!("Expected a GitHub repository");
+        };
+        assert_eq!(repo.to_string(), "Owner/Repo");
+        assert_eq!(repo.as_str(), "http://github.com/Owner/Repo///");
+        for source in [
+            "https://gitlab.com/owner/repo",
+            "https://github.com/owner/repo/tree/main",
+            "",
+        ] {
+            let yaml = format!("name: Example\nsource: '{source}'\n");
+            let tool: ToolEntry = serde_saphyr::from_str(&yaml)?;
+            assert!(matches!(tool.source, Some(ToolSource::Other(ref value)) if value == source));
+        }
+        for yaml in ["name: Example\n", "name: Example\nsource: null\n"] {
+            assert!(serde_saphyr::from_str::<ToolEntry>(yaml)?.source.is_none());
+        }
+        Ok(())
+    }
+
     #[test]
     fn tool_yaml_keeps_optional_fields_and_ignores_unrelated_metadata() -> Result<()> {
         let tool: ToolEntry =

ci/crates/pr-check/src/input.rs

@@ -2,7 +2,7 @@
 
 use anyhow::{Context, Result, bail};
 use chrono::{DateTime, Utc};
-use github_repo::GithubRepo;
+use github_repo::{GithubRepo, ToolSource};
 use serde::{Deserialize, de::DeserializeOwned};
 
 use crate::checks::{Check, DomainAge};
@@ -67,7 +67,7 @@ impl GithubClient {
     /// # Errors
     ///
     /// Returns an error if the API call fails.
-    async fn repo_info(&self, repo: GithubRepo<'_>) -> Result<Option<RepoInfo>> {
+    async fn repo_info(&self, repo: &GithubRepo) -> Result<Option<RepoInfo>> {
         let url = format!("https://api.github.com/repos/{repo}");
         self.get::<RepoInfo>(&url).await
     }
@@ -77,7 +77,7 @@ impl GithubClient {
     /// # Errors
     ///
     /// Returns an error if the API call fails.
-    async fn contributor_count(&self, repo: GithubRepo<'_>) -> Result<Option<usize>> {
+    async fn contributor_count(&self, repo: &GithubRepo) -> Result<Option<usize>> {
         let url = format!("https://api.github.com/repos/{repo}/contributors?per_page=100&anon=0");
         Ok(self
             .get::<Vec<Contributor>>(&url)
@@ -95,11 +95,7 @@ impl GithubClient {
 pub async fn check_tool(client: &GithubClient, tool: &ToolEntry) -> Result<ToolReport> {
     let source = &tool.source;
 
-    let repo = source
-        .as_deref()
-        .and_then(|url| GithubRepo::try_from(url).ok());
-
-    if let Some(repo) = repo {
+    if let Some(ToolSource::Github(repo)) = source {
         let repo_result = client.repo_info(repo).await;
         let contributors_result = client.contributor_count(repo).await;
 

ci/crates/pr-check/src/network.rs

@@ -1,6 +1,7 @@
 //! Report status, Markdown rendering, and workflow exit-code contract.
 
 use askama::Template;
+use github_repo::ToolSource;
 use std::process::ExitCode;
 
 // Identifies the report as output from the contribution checker.
@@ -42,7 +43,7 @@ impl CheckResult {
 #[derive(Debug)]
 pub struct ToolReport {
     pub name: String,
-    pub source: Option<String>,
+    pub source: Option<ToolSource>,
     pub stars: CheckResult,
     pub contributors: CheckResult,
     pub age: CheckResult,
@@ -140,6 +141,26 @@ mod tests {
         }
     }
 
+    #[test]
+    fn comments_preserve_source_text_for_each_variant() {
+        for source in [
+            "http://github.com/Owner/Repo///",
+            "https://gitlab.com/owner/repo",
+            "",
+        ] {
+            let report = ToolReport {
+                source: Some(source.into()),
+                ..passing_report()
+            };
+            let reports = Reports::from_iter([report]);
+            assert!(
+                Comment::from(&reports)
+                    .to_string()
+                    .contains(&format!("Source: {source}\n"))
+            );
+        }
+    }
+
     #[test]
     fn collect_and_extend_preserve_input_order() {
         let report = |name: &str| ToolReport {