From 54b8122f6610f15901f4310c4baefae91ff94057 Mon Sep 17 00:00:00 2001 From: damocles Date: Tue, 13 Oct 2026 14:58:43 +0200 Subject: [PATCH 1/3] repo-search: warn when a result count hits --limit, may be truncated --- docs/tools/forge.md | 8 +++++ hive-forge/src/verbs/repo_search.rs | 49 +++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/docs/tools/forge.md b/docs/tools/forge.md index efdefd7d..62d7a81f 100644 --- a/docs/tools/forge.md +++ b/docs/tools/forge.md @@ -341,4 +341,12 @@ to discover valid label names before triaging or to audit the label set. reports the real total from the response's `X-Total-Count` (`… 50 of 187 shown … 137 more`), so trust the trailer, not the row count, when deciding whether you've seen everything. +- `repo-search` has no `X-Total-Count` to fall back on, so it can only + warn, not quantify: a result count that exactly equals `--limit` + prints a stderr warning that more may exist, same trust-the-trailer + principle as `list` above but without a real number behind it. It + also can't tell "doesn't exist" from "you lack permission to see + it" — both render as an absent row with no signal either way — and + it matches repo **name** only, not owner, so there's no way to + enumerate an org's repos through this verb. diff --git a/hive-forge/src/verbs/repo_search.rs b/hive-forge/src/verbs/repo_search.rs index 5ad952c8..68fb2372 100644 --- a/hive-forge/src/verbs/repo_search.rs +++ b/hive-forge/src/verbs/repo_search.rs @@ -7,6 +7,14 @@ //! //! Human output: one `owner/repo description` line per hit. `--json` //! emits the full `SearchResults.data` array as raw JSON. +//! +//! A result count that exactly equals `--limit` is a hit on the cap, not +//! necessarily the whole answer — the forge may be holding back more (it +//! also silently drops repos the caller lacks permission to see, with no +//! distinguishing signal at all). Either way we warn on stderr rather than +//! let a capped list read as complete; stdout — and `--json` stdout +//! especially — stays exactly the raw result set, so a script piping it +//! doesn't have to parse prose out of its own data. use anyhow::Result; use clap::Args as ClapArgs; @@ -65,6 +73,9 @@ pub fn run(client: &Client, args: Args) -> Result<()> { .send()?; let repos = results.data.unwrap_or_default(); + if let Some(msg) = truncation_warning(repos.len() as u64, args.limit) { + eprintln!("{msg}"); + } if client.json_mode() { return print_json(&serde_json::to_value(&repos)?); } @@ -83,3 +94,41 @@ pub fn run(client: &Client, args: Args) -> Result<()> { } Ok(()) } + +/// A result count that exactly equals `--limit` is a hit on the cap, not +/// necessarily the whole answer — see the module doc. Returns the stderr +/// warning to print, or `None` when the count came in under the cap (a +/// short result *is* the complete answer, same reasoning `list`'s own +/// `trailer()` uses for its full-page heuristic). +fn truncation_warning(count: u64, limit: u64) -> Option { + (count == limit).then(|| { + format!( + "warning: got exactly --limit ({limit}) results — the forge may be holding back \ + more (and silently drops any repo you lack permission to see, with no signal \ + either way); raise --limit or narrow --query rather than trusting this as a \ + complete list" + ) + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn warns_when_count_hits_the_cap() { + assert!(truncation_warning(30, 30).is_some()); + } + + #[test] + fn stays_quiet_under_the_cap() { + assert_eq!(truncation_warning(9, 30), None); + } + + #[test] + fn stays_quiet_on_zero_results() { + // An empty result needs "(no results)" (printed separately in + // `run`), not a truncation warning implying more might exist. + assert_eq!(truncation_warning(0, 30), None); + } +} From acb5b676d76d91138ab6f45498794d55675b2187 Mon Sep 17 00:00:00 2001 From: damocles Date: Wed, 14 Oct 2026 15:57:39 +0200 Subject: [PATCH 2/3] repo-search: read X-Total-Count so the warning fires at the server's own clamp --- docs/tools/forge.md | 11 ++- hive-forge/src/verbs/repo_search.rs | 142 ++++++++++++++++++++++------ 2 files changed, 118 insertions(+), 35 deletions(-) diff --git a/docs/tools/forge.md b/docs/tools/forge.md index 62d7a81f..60987899 100644 --- a/docs/tools/forge.md +++ b/docs/tools/forge.md @@ -341,11 +341,12 @@ to discover valid label names before triaging or to audit the label set. reports the real total from the response's `X-Total-Count` (`… 50 of 187 shown … 137 more`), so trust the trailer, not the row count, when deciding whether you've seen everything. -- `repo-search` has no `X-Total-Count` to fall back on, so it can only - warn, not quantify: a result count that exactly equals `--limit` - prints a stderr warning that more may exist, same trust-the-trailer - principle as `list` above but without a real number behind it. It - also can't tell "doesn't exist" from "you lack permission to see +- `repo-search` reads `X-Total-Count` the same way `list` does and warns + with the real remaining count when it's present (`… 50 of 187 shown … + 137 more exist`); without it (an older forge, a stripping proxy) it + falls back to the cruder `count == limit` heuristic, same + trust-the-trailer-not-the-row-count principle as `list` above. Either + way it can't tell "doesn't exist" from "you lack permission to see it" — both render as an absent row with no signal either way — and it matches repo **name** only, not owner, so there's no way to enumerate an org's repos through this verb. diff --git a/hive-forge/src/verbs/repo_search.rs b/hive-forge/src/verbs/repo_search.rs index 68fb2372..91a6eef2 100644 --- a/hive-forge/src/verbs/repo_search.rs +++ b/hive-forge/src/verbs/repo_search.rs @@ -8,21 +8,57 @@ //! Human output: one `owner/repo description` line per hit. `--json` //! emits the full `SearchResults.data` array as raw JSON. //! -//! A result count that exactly equals `--limit` is a hit on the cap, not -//! necessarily the whole answer — the forge may be holding back more (it -//! also silently drops repos the caller lacks permission to see, with no -//! distinguishing signal at all). Either way we warn on stderr rather than -//! let a capped list read as complete; stdout — and `--json` stdout +//! Like `list`, the forge clamps the page size to its own +//! `api.MAX_RESPONSE_ITEMS` (50 by default), so a large `--limit` silently +//! returns a smaller page. Unlike `list`'s generated binding, this crate's +//! `repo_search` method does not expose the response headers — but +//! Forgejo's own docs name `/repos/search` as the worked example for +//! `X-Total-Count`, so [`response_type`][forgejo_api::sync::Request::response_type] +//! reinterprets the response as `(TotalCount, SearchResults)` to read it +//! anyway, same number `list`'s trailer reports. When the header is +//! missing or unparseable (an older forge, a stripping proxy) we fall back +//! to the cruder `count == limit` heuristic, which under-reports exactly +//! when the server clamped — the same fallback-not-rule relationship +//! `list.rs`'s `trailer()` documents. Either way we warn on stderr rather +//! than let a capped list read as complete; stdout — and `--json` stdout //! especially — stays exactly the raw result set, so a script piping it //! doesn't have to parse prose out of its own data. +//! +//! The total (when present) still can't tell "doesn't exist" from "you +//! lack permission to see it" — a hidden repo is just absent from both the +//! count and the rows, with no signal either way. use anyhow::Result; use clap::Args as ClapArgs; -use forgejo_api::structs::RepoSearchQuery; +use forgejo_api::StructureError; +use forgejo_api::structs::{RepoSearchQuery, SearchResults}; use crate::client::Client; use crate::verbs::print_json; +/// `X-Total-Count`, read generically via `response_type` since this +/// crate's `repo_search` binding (unlike `issue_list_issues`'s) has no +/// headers-carrying `Response` type of its own. Mirrors the header's own +/// error handling (`forgejo_api`'s `IssueListHeaders`) exactly, so a +/// non-ASCII or unparseable header fails the request the same way there. +struct TotalCount(Option); + +impl TryFrom<&reqwest::header::HeaderMap> for TotalCount { + type Error = StructureError; + + fn try_from(map: &reqwest::header::HeaderMap) -> Result { + let total = map + .get("x-total-count") + .map(|v| -> Result<_, StructureError> { + let s = v.to_str().map_err(|_| StructureError::HeaderNotAscii)?; + s.parse::() + .map_err(|_| StructureError::HeaderParseFailed) + }) + .transpose()?; + Ok(Self(total)) + } +} + #[derive(ClapArgs)] pub struct Args { /// Keyword to search for (matches repo name by default; combine with @@ -48,7 +84,7 @@ pub struct Args { /// serialisation errors when emitting `--json` output. pub fn run(client: &Client, args: Args) -> Result<()> { let limit = u32::try_from(args.limit.min(u64::from(u32::MAX))).unwrap_or(u32::MAX); - let results = client + let (TotalCount(total), results) = client .api() .repo_search(RepoSearchQuery { q: args.query, @@ -70,10 +106,11 @@ pub fn run(client: &Client, args: Args) -> Result<()> { order: None, }) .page_size(limit) + .response_type::<(TotalCount, SearchResults)>() .send()?; let repos = results.data.unwrap_or_default(); - if let Some(msg) = truncation_warning(repos.len() as u64, args.limit) { + if let Some(msg) = truncation_warning(total, repos.len() as u64, args.limit) { eprintln!("{msg}"); } if client.json_mode() { @@ -95,20 +132,45 @@ pub fn run(client: &Client, args: Args) -> Result<()> { Ok(()) } -/// A result count that exactly equals `--limit` is a hit on the cap, not -/// necessarily the whole answer — see the module doc. Returns the stderr -/// warning to print, or `None` when the count came in under the cap (a -/// short result *is* the complete answer, same reasoning `list`'s own -/// `trailer()` uses for its full-page heuristic). -fn truncation_warning(count: u64, limit: u64) -> Option { - (count == limit).then(|| { - format!( - "warning: got exactly --limit ({limit}) results — the forge may be holding back \ - more (and silently drops any repo you lack permission to see, with no signal \ - either way); raise --limit or narrow --query rather than trusting this as a \ - complete list" - ) - }) +/// Tell the caller, on stderr, whether this page is the whole answer. +/// +/// `total` is `X-Total-Count` when the header parsed; `repo-search` always +/// starts from page 1 (it has no `--page`, unlike `list`), so `count` alone +/// is how much of `total` has been shown — no page-number multiply needed. +/// +/// Without a usable total, falls back to the cruder `count == limit` +/// heuristic: a hit on the cap, not necessarily the whole answer, but a +/// short result *is* the complete answer. This is the case the server's +/// own clamp (`api.MAX_RESPONSE_ITEMS`, 50 by default) makes silently +/// wrong — `--limit 400` returning a clamped 50 reads as complete here, +/// which is exactly why the header-based path above is the rule and this +/// is only the fallback (same relationship as `list.rs`'s `trailer()`). +fn truncation_warning(total: Option, count: u64, limit: u64) -> Option { + if count == 0 { + // "(no results)" is printed separately in `run` — a truncation + // warning here would wrongly imply more might still exist. + return None; + } + match total { + Some(total) => { + let remaining = total.saturating_sub(count); + (remaining > 0).then(|| { + format!( + "warning: {count} of {total} results shown — {remaining} more exist (the \ + forge still silently drops any repo you lack permission to see, with no \ + signal either way); raise --limit or narrow --query to see them" + ) + }) + } + None => (count == limit).then(|| { + format!( + "warning: got exactly --limit ({limit}) results — the forge may be holding back \ + more (and silently drops any repo you lack permission to see, with no signal \ + either way); raise --limit or narrow --query rather than trusting this as a \ + complete list" + ) + }), + } } #[cfg(test)] @@ -116,19 +178,39 @@ mod tests { use super::*; #[test] - fn warns_when_count_hits_the_cap() { - assert!(truncation_warning(30, 30).is_some()); + fn warns_with_the_real_remaining_count_when_the_total_is_known() { + let msg = truncation_warning(Some(187), 30, 30).expect("137 more exist"); + assert!(msg.contains("30 of 187"), "{msg}"); + assert!(msg.contains("157 more"), "{msg}"); } #[test] - fn stays_quiet_under_the_cap() { - assert_eq!(truncation_warning(9, 30), None); + fn catches_the_clamp_the_old_count_eq_limit_heuristic_missed() { + // The regression this header read exists for: `--limit 400` against + // a forge that clamps to 50, with 187 matches. `count != limit` + // (50 != 400), so the old heuristic alone stayed silent here. + let msg = truncation_warning(Some(187), 50, 400).expect("137 more exist"); + assert!(msg.contains("50 of 187"), "{msg}"); + assert!(msg.contains("137 more"), "{msg}"); } #[test] - fn stays_quiet_on_zero_results() { - // An empty result needs "(no results)" (printed separately in - // `run`), not a truncation warning implying more might exist. - assert_eq!(truncation_warning(0, 30), None); + fn stays_quiet_once_the_total_is_accounted_for() { + assert_eq!(truncation_warning(Some(30), 30, 30), None); + } + + #[test] + fn falls_back_to_the_cap_heuristic_without_a_usable_total() { + assert!(truncation_warning(None, 30, 30).is_some()); + assert_eq!(truncation_warning(None, 9, 30), None); + } + + #[test] + fn stays_quiet_on_zero_results_even_with_a_total() { + // Permission-filtering can make count 0 while total is nonzero + // (every match is hidden from this caller) — still not a + // truncation warning's job; "(no results)" covers it in `run`. + assert_eq!(truncation_warning(Some(5), 0, 30), None); + assert_eq!(truncation_warning(None, 0, 30), None); } } From c335bc09628f8667a3d408768f49901044b0a224 Mon Sep 17 00:00:00 2001 From: damocles Date: Wed, 14 Oct 2026 18:34:01 +0200 Subject: [PATCH 3/3] repo-search: don't tell the caller to raise --limit once the forge has already clamped --- docs/tools/forge.md | 15 ++++++++---- hive-forge/src/verbs/repo_search.rs | 36 ++++++++++++++++++++++++++++- 2 files changed, 45 insertions(+), 6 deletions(-) diff --git a/docs/tools/forge.md b/docs/tools/forge.md index 60987899..8ad403cf 100644 --- a/docs/tools/forge.md +++ b/docs/tools/forge.md @@ -345,9 +345,14 @@ to discover valid label names before triaging or to audit the label set. with the real remaining count when it's present (`… 50 of 187 shown … 137 more exist`); without it (an older forge, a stripping proxy) it falls back to the cruder `count == limit` heuristic, same - trust-the-trailer-not-the-row-count principle as `list` above. Either - way it can't tell "doesn't exist" from "you lack permission to see - it" — both render as an absent row with no signal either way — and - it matches repo **name** only, not owner, so there's no way to - enumerate an org's repos through this verb. + trust-the-trailer-not-the-row-count principle as `list` above. Unlike + `list`, this verb has no `--page` — so once the forge's own page cap + has clamped the result (`count < limit`), raising `--limit` again asks + for the same clamp and gets nothing new; the warning says to narrow + `--query` instead in that case, and only suggests raising `--limit` + while still under the cap. Either way it can't tell "doesn't exist" + from "you lack permission to see it" — both render as an absent row + with no signal either way — and it matches repo **name** only, not + owner, so there's no way to enumerate an org's repos through this + verb. diff --git a/hive-forge/src/verbs/repo_search.rs b/hive-forge/src/verbs/repo_search.rs index 91a6eef2..8b492481 100644 --- a/hive-forge/src/verbs/repo_search.rs +++ b/hive-forge/src/verbs/repo_search.rs @@ -155,10 +155,23 @@ fn truncation_warning(total: Option, count: u64, limit: u64) -> Option { let remaining = total.saturating_sub(count); (remaining > 0).then(|| { + // `count < limit` means the forge clamped the page below what + // was asked for — raising `--limit` again asks for the same + // clamp and gets the same `count` back. Unlike `list`, this + // verb has no `--page` to walk past the clamp with, so + // narrowing `--query` is the only remedy that can work once + // the server has capped the page. "raise --limit" only + // helps the other case: still under the cap, where asking + // for more actually serves more of it. + let remedy = if count < limit { + "the forge capped this page at its own limit — narrow --query to see them" + } else { + "raise --limit or narrow --query to see them" + }; format!( "warning: {count} of {total} results shown — {remaining} more exist (the \ forge still silently drops any repo you lack permission to see, with no \ - signal either way); raise --limit or narrow --query to see them" + signal either way); {remedy}" ) }) } @@ -194,6 +207,27 @@ mod tests { assert!(msg.contains("137 more"), "{msg}"); } + #[test] + fn clamp_case_tells_the_caller_to_narrow_not_to_raise_limit() { + // `--limit` already exceeded the forge's own page cap here, so + // "raise --limit" is a dead end — only `--query` can still narrow + // the result set below the cap. (The bug argus caught: both + // branches used to say "raise --limit", which can't work once the + // server itself has clamped the page.) + let msg = truncation_warning(Some(187), 50, 400).expect("137 more exist"); + assert!(msg.contains("narrow --query"), "{msg}"); + assert!(!msg.contains("raise --limit"), "{msg}"); + } + + #[test] + fn under_the_cap_case_still_offers_raise_limit() { + // `count == limit` and both are under the forge's cap: asking for + // more can actually serve more, so "raise --limit" is live advice + // here, not a dead end. + let msg = truncation_warning(Some(187), 30, 30).expect("157 more exist"); + assert!(msg.contains("raise --limit"), "{msg}"); + } + #[test] fn stays_quiet_once_the_total_is_accounted_for() { assert_eq!(truncation_warning(Some(30), 30, 30), None);