From 09168da667f73edefe2319d1a11022dff7349114 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 4 Nov 2026 23:52:28 +0000 Subject: [PATCH 1/5] fix(idris-dependents): repair bot-generated tests that broke CI on main MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #628 merged with CodeRabbit's generated tests failing, so `build · test · clippy (idris-dependents)` has been red on main since: - scan_tests: `ready_mock` omitted the new `responses` field (E0063); fill the rest from `Mock::default()`. - scan_edge_tests: the report JSON contract lacked the new `skippedUrls`. - scan_edge_tests: the "hard failure" case used 404, which is now skipped by design; use 500 for the hard failure and assert the 404 skip (empty rows, URL listed in `skipped_urls`) separately. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01J4QvDfb1gn2SD218jMg9jk --- idris-dependents/tests/scan_edge_tests.rs | 20 ++++++++++++++++++-- idris-dependents/tests/scan_tests.rs | 5 +---- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/idris-dependents/tests/scan_edge_tests.rs b/idris-dependents/tests/scan_edge_tests.rs index 587bbc37..5f8b29c1 100644 --- a/idris-dependents/tests/scan_edge_tests.rs +++ b/idris-dependents/tests/scan_edge_tests.rs @@ -387,9 +387,24 @@ fn raw_content_fetches_retry_rate_limits_and_propagate_hard_failures() { false, vec![item("alice/app", "a.ipkg", "contents/a")], ); - io.respond("contents/a", true, Ok(response(404, &[], ""))); + io.respond("contents/a", true, Ok(response(500, &[], ""))); let error = run(&options(), &mut io).unwrap_err().to_string(); - assert!(error.contains("HTTP 404 for contents/a"), "{error}"); + 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(); } @@ -440,6 +455,7 @@ fn report_json_keeps_public_field_names_and_confidence_spelling() { "confidence": "MEDIUM", }], "incompleteQueries": [], + "skippedUrls": [], }) ); io.done(); diff --git a/idris-dependents/tests/scan_tests.rs b/idris-dependents/tests/scan_tests.rs index 81958f19..2249a1cc 100644 --- a/idris-dependents/tests/scan_tests.rs +++ b/idris-dependents/tests/scan_tests.rs @@ -350,11 +350,8 @@ fn non_rate_limit_errors_fail_loudly() { fn ready_mock() -> Mock { Mock { - searches: BTreeMap::new(), - contents: BTreeMap::new(), limited_once: true, - logs: vec![], - gets: 0, + ..Mock::default() } } From f7a93668ee4cbae4bc17ce239e594d9278213f6e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 4 Nov 2026 23:53:04 +0000 Subject: [PATCH 2/5] fix(idris-dependents): address Codex and CodeRabbit review findings from #628 Each fix has a regression test that fails on the previous code: - ipkg: an unterminated string now runs to end of input, so `brief = "depends = proven` no longer reports `proven` (Codex). - refs: GitHub must be the URL's host, not a path segment, so `https://example.com/mirrors/github.com//x` is not a HIGH match; ports, userinfo and `git+https` are handled (Codex). - refs: `.gitmodules` keys are case-insensitive (`URL =`), as in git config (Codex). - refs/scan: relative submodule URLs (`../..//repo.git`) are resolved against the dependent repository, as git does (Codex). - scan: HTTP errors include a bounded excerpt of GitHub's response body, e.g. "Bad credentials" for a 401 (CodeRabbit). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01J4QvDfb1gn2SD218jMg9jk --- idris-dependents/README.adoc | 2 +- idris-dependents/src/ipkg.rs | 12 ++- idris-dependents/src/refs.rs | 103 ++++++++++++++++------ idris-dependents/src/scan.rs | 9 +- idris-dependents/tests/rules_tests.rs | 69 ++++++++++++++- idris-dependents/tests/scan_edge_tests.rs | 24 +++++ 6 files changed, 179 insertions(+), 40 deletions(-) diff --git a/idris-dependents/README.adoc b/idris-dependents/README.adoc index 81f829ac..efd1b454 100644 --- a/idris-dependents/README.adoc +++ b/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 |=== diff --git a/idris-dependents/src/ipkg.rs b/idris-dependents/src/ipkg.rs index 2c16a310..663c8a31 100644 --- a/idris-dependents/src/ipkg.rs +++ b/idris-dependents/src/ipkg.rs @@ -108,13 +108,11 @@ pub fn lex(src: &str) -> (Vec, 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)) { diff --git a/idris-dependents/src/refs.rs b/idris-dependents/src/refs.rs index 5e87859f..c29d1a12 100644 --- a/idris-dependents/src/refs.rs +++ b/idris-dependents/src/refs.rs @@ -11,32 +11,62 @@ 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 { + let url = url.trim().to_ascii_lowercase(); + let (authority, path) = match url.find("://") { + Some(i) => { + let rest = &url[i + 3..]; + let slash = rest.find('/')?; + (&rest[..slash], &rest[slash + 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| { + let mut parts = path.splitn(2, '/'); + 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/`. Absolute URLs are returned unchanged. +fn resolve_submodule_url(url: &str, superproject: Option<&str>) -> Option { + if !(url.starts_with("../") || url.starts_with("./")) { + return Some(url.to_string()); + } + let mut base: Vec<&str> = 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://github.com/{}", base.join("/"))) } /// `pack.toml` custom entries pointing at `owners`, in file order. @@ -68,20 +98,35 @@ pub fn pack_toml_refs(text: &str, owners: &[String]) -> Result, 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 { + 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 { 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() } diff --git a/idris-dependents/src/scan.rs b/idris-dependents/src/scan.rs index 7117a64f..8f9571e8 100644 --- a/idris-dependents/src/scan.rs +++ b/idris-dependents/src/scan.rs @@ -27,7 +27,12 @@ pub struct Response { impl Response { fn into_body(self, url: &str) -> Result { 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(opts: &Options, io: &mut I) -> Result { } }, 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 diff --git a/idris-dependents/tests/rules_tests.rs b/idris-dependents/tests/rules_tests.rs index a4d06633..e9240a1a 100644 --- a/idris-dependents/tests/rules_tests.rs +++ b/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,70 @@ 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()); +} diff --git a/idris-dependents/tests/scan_edge_tests.rs b/idris-dependents/tests/scan_edge_tests.rs index 5f8b29c1..5f4437b6 100644 --- a/idris-dependents/tests/scan_edge_tests.rs +++ b/idris-dependents/tests/scan_edge_tests.rs @@ -460,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(); +} From f6c367550011fc8d31d4e028aca58958d6ad67d1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 00:59:26 +0000 Subject: [PATCH 3/5] fix(idris-dependents): resolve relative submodule URLs past owner/repo like git Codex on #644: git resolves a relative submodule URL against the whole remote URL, so from alice/app `../../../github.com/hyperpolymath/proven.git` is https://github.com/hyperpolymath/proven.git; the resolver stopped at the owner/repo boundary and dropped it. Model the remote as github.com// so `../` can climb into the host part. The new test's expected values are what `git submodule init` (git 2.43) registers for a superproject with origin https://github.com/alice/app.git. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01J4QvDfb1gn2SD218jMg9jk --- idris-dependents/src/refs.rs | 9 +++++++-- idris-dependents/tests/rules_tests.rs | 25 +++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/idris-dependents/src/refs.rs b/idris-dependents/src/refs.rs index c29d1a12..9b164c0f 100644 --- a/idris-dependents/src/refs.rs +++ b/idris-dependents/src/refs.rs @@ -49,11 +49,16 @@ pub fn points_at(url: &str, owners: &[String]) -> bool { /// 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/`. 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 { if !(url.starts_with("../") || url.starts_with("./")) { return Some(url.to_string()); } - let mut base: Vec<&str> = superproject?.split('/').filter(|s| !s.is_empty()).collect(); + 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("../") { @@ -66,7 +71,7 @@ fn resolve_submodule_url(url: &str, superproject: Option<&str>) -> Option Date: Thu, 5 Nov 2026 01:05:25 +0000 Subject: [PATCH 4/5] fix(idris-dependents): owner pages with a query or fragment are not repositories Codex on #644: `https://github.com/hyperpolymath/?tab=repositories` (or `/#about`) made `?tab=repositories` the "repository", so an org-page URL in pack.toml or .gitmodules counted as a HIGH-confidence dependent. Strip the query and fragment before splitting owner/repo. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01J4QvDfb1gn2SD218jMg9jk --- idris-dependents/src/refs.rs | 4 +++- idris-dependents/tests/rules_tests.rs | 23 +++++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/idris-dependents/src/refs.rs b/idris-dependents/src/refs.rs index 9b164c0f..ea6dfc8b 100644 --- a/idris-dependents/src/refs.rs +++ b/idris-dependents/src/refs.rs @@ -38,7 +38,9 @@ fn github_path(url: &str) -> Option { /// 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| { - let mut parts = path.splitn(2, '/'); + // 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; }; diff --git a/idris-dependents/tests/rules_tests.rs b/idris-dependents/tests/rules_tests.rs index 2ee41704..0fa8a089 100644 --- a/idris-dependents/tests/rules_tests.rs +++ b/idris-dependents/tests/rules_tests.rs @@ -662,3 +662,26 @@ fn relative_submodule_urls_match_git_resolution() { 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 + )); +} From a4fabd346413bfb7dbdd0a8c5650848cbf74bbec Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 01:06:40 +0000 Subject: [PATCH 5/5] fix(idris-dependents): end the URL authority at `?` or `#`, not only `/` CodeRabbit on #644: `https://evil.example#@github.com/hyperpolymath/proven` took everything before the first `/` as the authority, so the host after `@` read as github.com. Per RFC 3986 the authority ends at the first `/`, `?` or `#`; a URL whose authority is not followed by `/` names no repository. Query/fragment text in the path was already dropped (9eccde1). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01J4QvDfb1gn2SD218jMg9jk --- idris-dependents/src/refs.rs | 9 +++++++-- idris-dependents/tests/rules_tests.rs | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/idris-dependents/src/refs.rs b/idris-dependents/src/refs.rs index ea6dfc8b..3fdfc199 100644 --- a/idris-dependents/src/refs.rs +++ b/idris-dependents/src/refs.rs @@ -18,8 +18,13 @@ fn github_path(url: &str) -> Option { let (authority, path) = match url.find("://") { Some(i) => { let rest = &url[i + 3..]; - let slash = rest.find('/')?; - (&rest[..slash], &rest[slash + 1..]) + // 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(':')?; diff --git a/idris-dependents/tests/rules_tests.rs b/idris-dependents/tests/rules_tests.rs index 0fa8a089..b0277d74 100644 --- a/idris-dependents/tests/rules_tests.rs +++ b/idris-dependents/tests/rules_tests.rs @@ -685,3 +685,23 @@ fn owner_pages_with_query_or_fragment_are_not_repositories() { &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 + )); +}