fix(idris-dependents): green CI on main + close #628 review findings

idris-dependents/README.adoc

@@ -34,7 +34,7 @@ cargo run -- parse-ipkg path/to/pkg.ipkg      # offline: name + depends as JSON
 +
 [cols="1,4"]
 |===
-|HIGH   |`pack.toml` or `.gitmodules` points at one of our GitHub owners
+|HIGH   |`pack.toml` or `.gitmodules` points at one of our GitHub owners (GitHub must be the URL host; relative submodule URLs are resolved against the dependent repository)
 |MEDIUM |an `.ipkg` `depends` on a distinctive package name of ours
 |LOW    |only ambiguous names (`proven`, `trope`) with no corroboration
 |===

idris-dependents/src/ipkg.rs

@@ -108,13 +108,11 @@ pub fn lex(src: &str) -> (Vec<Tok>, usize) {
             continue;
         }
         if c == '"' {
-            if let Some(end) = string_end(&s, i) {
-                toks.push(Tok::Str(s[i + 1..end - 1].iter().collect()));
-                i = end;
-            } else {
-                skipped += 1;
-                i += 1;
-            }
+            // An unterminated string runs to end of input, so its contents are
+            // never read as package syntax.
+            let end = string_end(&s, i).unwrap_or(s.len() + 1);
+            toks.push(Tok::Str(s[i + 1..end - 1].iter().collect()));
+            i = end;
             continue;
         }
         let (tok, len) = match (c, at(i + 1)) {

idris-dependents/src/refs.rs

@@ -11,32 +11,74 @@ pub struct PackRef {
     pub url: String,
 }
 
-/// Match a watched owner in a GitHub-style repository reference, including
-/// HTTPS, SSH, and scp forms. Owner matching is ASCII case-insensitive and
-/// requires a following `/`.
-///
-/// This is a textual check, not full URL validation: `github.com` may follow
-/// `/`, `@`, or `.` (or start the input), so subdomains and embedded paths can match.
-pub fn points_at(url: &str, owners: &[String]) -> bool {
-    let url = url.to_ascii_lowercase();
-    let mut from = 0;
-    while let Some(k) = url[from..].find("github.com") {
-        let at = from + k;
-        let host_start = at == 0 || matches!(url.as_bytes()[at - 1], b'/' | b'@' | b'.');
-        let rest = &url[at + "github.com".len()..];
-        if host_start && (rest.starts_with('/') || rest.starts_with(':')) {
-            let path = &rest[1..];
-            if owners.iter().any(|o| {
-                let o = o.to_ascii_lowercase();
-                path.strip_prefix(o.as_str())
-                    .is_some_and(|r| r.starts_with('/'))
-            }) {
-                return true;
+/// The repository path of a GitHub URL (`scheme://[user@]host[:port]/path` or
+/// scp-style `[user@]host:path`), lower-cased, when the host is GitHub itself.
+fn github_path(url: &str) -> Option<String> {
+    let url = url.trim().to_ascii_lowercase();
+    let (authority, path) = match url.find("://") {
+        Some(i) => {
+            let rest = &url[i + 3..];
+            // The authority ends at the first `/`, `?` or `#` (RFC 3986), so
+            // `https://evil.example#@github.com/...` has host `evil.example`.
+            let end = rest.find(['/', '?', '#'])?;
+            if !rest[end..].starts_with('/') {
+                return None;
+            }
+            (&rest[..end], &rest[end + 1..])
+        }
+        None => {
+            let colon = url.find(':')?;
+            if url[..colon].contains('/') {
+                return None;
             }
+            (&url[..colon], url[colon + 1..].trim_start_matches('/'))
+        }
+    };
+    let host = authority.rsplit('@').next()?.split(':').next()?;
+    matches!(host, "github.com" | "www.github.com").then(|| path.to_string())
+}
+
+/// Match a watched owner in a GitHub repository URL (HTTPS, SSH or scp form).
+/// GitHub must be the URL's host, not merely appear in its path, and the owner
+/// must be followed by a repository name. Matching is ASCII case-insensitive.
+pub fn points_at(url: &str, owners: &[String]) -> bool {
+    github_path(url).is_some_and(|path| {
+        // A query or fragment is not part of the repository path.
+        let path = path.split(['?', '#']).next().unwrap_or("");
+        let mut parts = path.split('/');
+        let (Some(owner), Some(repo)) = (parts.next(), parts.next()) else {
+            return false;
+        };
+        !repo.is_empty() && owners.iter().any(|o| o.eq_ignore_ascii_case(owner))
+    })
+}
+
+/// Resolve a submodule URL relative to its superproject (`../x` and `./x`, as
+/// git does against the superproject's remote), taking the remote to be
+/// `https://github.com/<full_name>`. Absolute URLs are returned unchanged.
+/// Like git, `../` may climb past `owner/repo` into the host part, so
+/// `../../../github.com/o/r` from `a/b` is `https://github.com/o/r`; climbing
+/// past the host yields `None` (git's result there is never a GitHub URL).
+fn resolve_submodule_url(url: &str, superproject: Option<&str>) -> Option<String> {
+    if !(url.starts_with("../") || url.starts_with("./")) {
+        return Some(url.to_string());
+    }
+    let mut base: Vec<&str> = std::iter::once("github.com")
+        .chain(superproject?.split('/').filter(|s| !s.is_empty()))
+        .collect();
+    let mut rest = url;
+    loop {
+        if let Some(r) = rest.strip_prefix("../") {
+            base.pop()?;
+            rest = r;
+        } else if let Some(r) = rest.strip_prefix("./") {
+            rest = r;
+        } else {
+            break;
         }
-        from = at + 1;
     }
-    false
+    base.push(rest);
+    Some(format!("https://{}", base.join("/")))
 }
 
 /// `pack.toml` custom entries pointing at `owners`, in file order.
@@ -68,20 +110,35 @@ pub fn pack_toml_refs(text: &str, owners: &[String]) -> Result<Vec<PackRef>, tom
 /// Extract URLs matching `owners` from trimmed `url =` lines, in input order.
 /// Double-quoted values end at the next quote; unquoted values end at `#` or
 /// `;`. Sections and escapes are not interpreted, and duplicates are retained.
+/// Relative URLs cannot be resolved without the superproject, so they are
+/// skipped; see [`gitmodules_refs_in`].
 pub fn gitmodules_refs(text: &str, owners: &[String]) -> Vec<String> {
+    gitmodules_refs_in(text, owners, None)
+}
+
+/// Like [`gitmodules_refs`], resolving relative submodule URLs against the
+/// GitHub repository `superproject` (`owner/repo`). Keys are matched
+/// case-insensitively, as git config does; relative matches are returned
+/// resolved to absolute URLs.
+pub fn gitmodules_refs_in(
+    text: &str,
+    owners: &[String],
+    superproject: Option<&str>,
+) -> Vec<String> {
     text.lines()
         .filter_map(|line| {
             let line = line.trim();
-            let value = line
-                .strip_prefix("url")?
-                .trim_start()
-                .strip_prefix('=')?
-                .trim();
+            let key = line.get(..3)?;
+            if !key.eq_ignore_ascii_case("url") {
+                return None;
+            }
+            let value = line[3..].trim_start().strip_prefix('=')?.trim();
             let value = match value.strip_prefix('"') {
                 Some(q) => q.split('"').next().unwrap_or(""),
                 None => value.split(['#', ';']).next().unwrap_or("").trim(),
             };
-            points_at(value, owners).then(|| value.to_string())
+            let url = resolve_submodule_url(value, superproject)?;
+            points_at(&url, owners).then_some(url)
         })
         .collect()
 }

idris-dependents/src/scan.rs

@@ -27,7 +27,12 @@ pub struct Response {
 impl Response {
     fn into_body(self, url: &str) -> Result<String> {
         if !(200..300).contains(&self.status) {
-            bail!("HTTP {} for {url}", self.status);
+            // GitHub puts the reason ("Bad credentials", secondary limits…) in the body.
+            let detail: String = self.body.trim().chars().take(300).collect();
+            if detail.is_empty() {
+                bail!("HTTP {} for {url}", self.status);
+            }
+            bail!("HTTP {} for {url}: {detail}", self.status);
         }
         Ok(self.body)
     }
@@ -325,7 +330,7 @@ pub fn run<I: Io>(opts: &Options, io: &mut I) -> Result<Report> {
                     }
                 },
                 Kind::Submodule => {
-                    let r = refs::gitmodules_refs(&text, &opts.owners);
+                    let r = refs::gitmodules_refs_in(&text, &opts.owners, Some(&it.full_name));
                     let found = !r.is_empty();
                     acc.sub_refs.extend(r);
                     found

idris-dependents/tests/rules_tests.rs

@@ -4,7 +4,7 @@
 
 use idris_dependents::catalogue::{classify, is_distinctive, search_stems, Confidence};
 use idris_dependents::ipkg::{depends, lex, package_name, Tok};
-use idris_dependents::refs::{gitmodules_refs, pack_toml_refs, points_at};
+use idris_dependents::refs::{gitmodules_refs, gitmodules_refs_in, pack_toml_refs, points_at};
 
 fn fx(name: &str) -> String {
     std::fs::read_to_string(format!(
@@ -570,3 +570,138 @@ url = ""
         ])
     );
 }
+
+// --- review findings on #628 (Codex, CodeRabbit) -------------------------------
+
+#[test]
+fn unterminated_string_hides_its_contents() {
+    // Codex: `brief = "depends = proven` must not report `proven`.
+    assert!(depends("package x\nbrief = \"depends = proven").is_empty());
+    assert_eq!(
+        depends("package x\ndepends = ochrance\nbrief = \"oops depends = proven"),
+        s(&["ochrance"])
+    );
+}
+
+#[test]
+fn github_must_be_the_host_not_a_path_segment() {
+    // Codex: a mirror path containing github.com is not a GitHub URL.
+    let o = own();
+    assert!(!points_at(
+        "https://example.com/mirrors/github.com/hyperpolymath/proven",
+        &o
+    ));
+    assert!(!points_at(
+        "git@example.com:github.com/hyperpolymath/proven",
+        &o
+    ));
+    assert!(points_at(
+        "ssh://git@github.com:22/hyperpolymath/proven.git",
+        &o
+    ));
+    assert!(points_at("git+https://github.com/hyperpolymath/proven", &o));
+    assert!(points_at(
+        "https://user@github.com/Hyperpolymath/proven",
+        &o
+    ));
+}
+
+#[test]
+fn gitmodules_keys_are_case_insensitive() {
+    // Codex: git config keys are case-insensitive (`URL =` is valid).
+    let t = "[submodule \"p\"]\n\tURL = https://github.com/hyperpolymath/proven.git\n\tUrl=https://github.com/metadatastician/x";
+    assert_eq!(
+        gitmodules_refs(t, &own()),
+        s(&[
+            "https://github.com/hyperpolymath/proven.git",
+            "https://github.com/metadatastician/x"
+        ])
+    );
+    assert!(gitmodules_refs("urlx = https://github.com/hyperpolymath/p", &own()).is_empty());
+}
+
+#[test]
+fn relative_submodule_urls_resolve_against_the_superproject() {
+    // Codex: `../../hyperpolymath/proven.git` from alice/app is hyperpolymath/proven.
+    let t = "[submodule \"p\"]\n\turl = ../../hyperpolymath/proven.git\n[submodule \"q\"]\n\turl = ../sibling.git";
+    assert_eq!(
+        gitmodules_refs_in(t, &own(), Some("alice/app")),
+        s(&["https://github.com/hyperpolymath/proven.git"])
+    );
+    // A sibling of a hyperpolymath fork's own repo resolves to that owner too.
+    assert_eq!(
+        gitmodules_refs_in("url = ./../proven", &own(), Some("hyperpolymath/app")),
+        s(&["https://github.com/hyperpolymath/proven"])
+    );
+    // Without the superproject, or climbing past the host, nothing is claimed.
+    assert!(gitmodules_refs(t, &own()).is_empty());
+    assert!(gitmodules_refs_in("url = ../../../x/y", &own(), Some("alice/app")).is_empty());
+}
+
+#[test]
+fn relative_submodule_urls_match_git_resolution() {
+    // Codex on #644. Expected values are what `git submodule init` (2.43)
+    // registers for a superproject whose origin is https://github.com/alice/app.git.
+    let all = |u: &str| {
+        gitmodules_refs_in(
+            &format!("url = {u}"),
+            &s(&["hyperpolymath", "alice"]),
+            Some("alice/app"),
+        )
+    };
+    assert_eq!(
+        all("../../../github.com/hyperpolymath/proven.git"),
+        s(&["https://github.com/hyperpolymath/proven.git"])
+    );
+    assert_eq!(
+        all("../../hyperpolymath/proven.git"),
+        s(&["https://github.com/hyperpolymath/proven.git"])
+    );
+    assert_eq!(all("./../proven"), s(&["https://github.com/alice/proven"]));
+    // git: https://x/y, https:/too/far — never a GitHub URL.
+    assert!(all("../../../x/y").is_empty());
+    assert!(all("../../../../too/far").is_empty());
+}
+
+#[test]
+fn owner_pages_with_query_or_fragment_are_not_repositories() {
+    // Codex on #644: a profile URL names no repository.
+    let o = own();
+    assert!(!points_at(
+        "https://github.com/hyperpolymath/?tab=repositories",
+        &o
+    ));
+    assert!(!points_at("https://github.com/hyperpolymath/#about", &o));
+    assert!(!points_at(
+        "https://github.com/hyperpolymath?tab=repositories",
+        &o
+    ));
+    assert!(points_at(
+        "https://github.com/hyperpolymath/proven#readme",
+        &o
+    ));
+    assert!(points_at(
+        "https://github.com/hyperpolymath/proven/tree/main?x=1",
+        &o
+    ));
+}
+
+#[test]
+fn authority_ends_at_query_or_fragment() {
+    // CodeRabbit on #644: the host here is evil.example, not github.com.
+    let o = own();
+    assert!(!points_at(
+        "https://evil.example#@github.com/hyperpolymath/proven",
+        &o
+    ));
+    assert!(!points_at(
+        "https://evil.example?@github.com/hyperpolymath/proven",
+        &o
+    ));
+    assert!(!points_at("https://github.com?x=/hyperpolymath/proven", &o));
+    assert!(!points_at("https://github.com/hyperpolymath/#/proven", &o));
+    assert!(points_at(
+        "https://user@github.com/hyperpolymath/proven",
+        &o
+    ));
+}

idris-dependents/tests/scan_edge_tests.rs

@@ -391,6 +391,21 @@ fn raw_content_fetches_retry_rate_limits_and_propagate_hard_failures() {
     let error = run(&options(), &mut io).unwrap_err().to_string();
     assert!(error.contains("HTTP 500 for contents/a"), "{error}");
     io.done();
+
+    // A file deleted since GitHub indexed it is skipped, not fatal.
+    let mut io = Script::default();
+    io.search(
+        QUERY,
+        1,
+        1,
+        false,
+        vec![item("alice/app", "a.ipkg", "contents/a")],
+    );
+    io.respond("contents/a", true, Ok(response(404, &[], "")));
+    let report = run(&options(), &mut io).unwrap();
+    assert!(report.rows.is_empty());
+    assert_eq!(report.skipped_urls, vec!["contents/a"]);
+    io.done();
 }
 
 #[test]
@@ -445,3 +460,27 @@ fn report_json_keeps_public_field_names_and_confidence_spelling() {
     );
     io.done();
 }
+
+#[test]
+fn http_errors_carry_github_reason() {
+    // CodeRabbit: surface the body (e.g. "Bad credentials"), not just the status.
+    let mut io = Script::default();
+    io.search(
+        QUERY,
+        1,
+        1,
+        false,
+        vec![item("alice/app", "a.ipkg", "contents/a")],
+    );
+    io.respond(
+        "contents/a",
+        true,
+        Ok(response(500, &[], "{\"message\":\"Bad credentials\"}")),
+    );
+    let error = run(&options(), &mut io).unwrap_err().to_string();
+    assert!(
+        error.contains("HTTP 500 for contents/a: {\"message\":\"Bad credentials\"}"),
+        "{error}"
+    );
+    io.done();
+}