From 8e690c06946983586ef4ab98671296a7047e6a6a Mon Sep 17 00:00:00 2001 From: atlas Date: Wed, 5 Aug 2026 18:26:34 +0200 Subject: [PATCH] feat(forge): search + milestone filters, and a page trailer that can't lie MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `list` already built its query with `q: None, milestones: None` — both fields were on the request it was sending. So full-text search over title and body is a flag, not a new verb, and a text match is only useful composed with the other filters anyway. The trailer was the real defect. It fired on `count == limit`, but the forge clamps page size to its own `api.MAX_RESPONSE_ITEMS`: ask for 400, get a full 50, and `50 != 400` kept it silent — suppressing the warning in precisely the case where the truncation is invisible. It now reports the real total from `X-Total-Count`, which the response header struct already parsed and the call site discarded. The requested limit is not clamped client-side: that ceiling is the remote's configuration, not ours. --- docs/tools/forge.md | 14 ++++ hive-forge/src/verbs/list.rs | 138 ++++++++++++++++++++++++++++++----- 2 files changed, 135 insertions(+), 17 deletions(-) diff --git a/docs/tools/forge.md b/docs/tools/forge.md index 8de53cc6..14d8ca28 100644 --- a/docs/tools/forge.md +++ b/docs/tools/forge.md @@ -67,6 +67,8 @@ hive-forge diff 42 # unified diff (lockfile hunks colla hive-forge diff 42 --full # include unfiltered lockfile hunks hive-forge list # open issues/PRs hive-forge list --kind pr --state all --page 2 # page 2 of all PRs (walk --page 1,2,… with --limit as page size for a repo-wide sweep) +hive-forge list --search "trust bundle" --state all # full-text over title AND body — the duplicate check +hive-forge list --milestone 11 --state all # what's left in a milestone (name or id, repeatable) hive-forge milestone # list milestones hive-forge branches deployed/ # filter branches by pattern hive-forge tree-sha main # git tree SHA for a ref @@ -282,3 +284,15 @@ to discover valid label names before triaging or to audit the label set. PR number is parsed back out of the push output (the AGit push itself has no label field), so they're silently skipped if that parse fails — same fallback as the deferred multi-line body. +- `list --milestone ` takes a milestone **name or id**, is + repeatable, and the forge *discards* one it doesn't recognise. So a + typo returns the **unfiltered** list rather than an empty one — the + failure looks like "this milestone contains everything", not like an + error. Confirm the spelling with `hive-forge milestone`. (Same + silent-discard shape as unknown labels above.) +- `list --limit N` is a *request*: the forge clamps page size to its own + `api.MAX_RESPONSE_ITEMS` (50 by default), so `--limit 400` returns at + most 50 rows. The stderr trailer 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. diff --git a/hive-forge/src/verbs/list.rs b/hive-forge/src/verbs/list.rs index a3da678c..91cb386f 100644 --- a/hive-forge/src/verbs/list.rs +++ b/hive-forge/src/verbs/list.rs @@ -1,5 +1,6 @@ //! `list [--kind issue|pr|both] [--state open|closed|all] [--assignee -//! ] [--author ] [--label ] [--limit N] [--page N]` — +//! ] [--author ] [--label ] [--milestone ] +//! [--search ] [--limit N] [--page N]` — //! list issues / PRs with filters. Pretty `#NNN [author] title` output by //! default; `--json` for piping. `--limit` is the page size and `--page` //! the 1-based page number — walk pages incrementally for a large result @@ -9,6 +10,11 @@ //! filters one-for-one so the mental model carries over. Closes the //! read-side curl-fallback gap (no boundary concerns — //! every agent + the operator queries the issue tracker constantly). +//! +//! `--search` is why there is no separate `search` verb: the forge +//! matches it against title **and body** on this same endpoint, and a +//! text match is only useful *composed* with the other filters — a +//! separate verb would have to grow every one of them back. use anyhow::Result; use clap::{Args as ClapArgs, ValueEnum}; @@ -85,8 +91,22 @@ pub struct Args { /// Filter to items carrying any of these label names. Repeatable. #[arg(long = "label")] labels: Vec, - /// Page size — items per page (default: 30; forge's per-page cap, - /// ~50, applies). Must be >= 1. + /// Filter to items in any of these milestones, by name or id. + /// Repeatable. A name that doesn't exist is discarded by the forge + /// rather than rejected — so a typo returns the UNFILTERED list, not + /// an empty one. Check the spelling against `milestone list`. + #[arg(long = "milestone")] + milestones: Vec, + /// Full-text search over title AND body, server-side. Composes with + /// every filter above — this is the duplicate-hunting path that + /// grepping `list` output can't cover, since grep only ever sees + /// the titles. + #[arg(long)] + search: Option, + /// Page size — items per page (default: 30). The forge clamps this + /// to its own `api.MAX_RESPONSE_ITEMS` (50 by default), so a large + /// `--limit` silently returns a smaller page; the trailer reports + /// the real total rather than trusting this number. Must be >= 1. #[arg(long, default_value_t = 30, value_parser = clap::value_parser!(u64).range(1..))] limit: u64, /// Page number to fetch (1-based, default 1). Combine with `--limit` @@ -101,9 +121,10 @@ pub fn run(client: &Client, args: Args) -> Result<()> { state: Some(args.state.query_state()), // The forge parses `labels` as a comma-separated list of names. labels: (!args.labels.is_empty()).then(|| args.labels.join(",")), - q: None, + q: args.search.clone().filter(|s| !s.is_empty()), r#type: args.kind.query_type(), - milestones: None, + // Same comma-separated shape as `labels` above. + milestones: (!args.milestones.is_empty()).then(|| args.milestones.join(",")), since: None, before: None, created_by: args.author.clone().filter(|s| !s.is_empty()), @@ -111,7 +132,7 @@ pub fn run(client: &Client, args: Args) -> Result<()> { mentioned_by: args.mention.clone().filter(|s| !s.is_empty()), sort: None, }; - let (_, issues) = client + let (headers, issues) = client .api() .issue_list_issues(owner, name, query) .page(u32::try_from(args.page).unwrap_or(u32::MAX)) @@ -125,21 +146,63 @@ pub fn run(client: &Client, args: Args) -> Result<()> { for item in items.as_array().into_iter().flatten() { print_row(item); } - // When the returned page is exactly `--limit` items, more pages - // may exist. Print a hint to stderr so the caller knows to fetch - // the next page rather than assuming the result is complete. Only - // fires on a full page — a short or empty page signals the end. - if count == args.limit { - eprintln!( - "… {} shown (page {}); more results may exist — re-run with --page {} (or raise --limit).", - args.limit, - args.page, - args.page + 1 - ); + if let Some(msg) = trailer( + headers.x_total_count.and_then(|t| u64::try_from(t).ok()), + count, + args.page, + args.limit, + ) { + eprintln!("{msg}"); } Ok(()) } +/// Tell the caller, on stderr, whether this page is the whole answer. +/// +/// The forge clamps `page_size` to its own `api.MAX_RESPONSE_ITEMS`, so +/// the requested `limit` is not the page size that was served: with +/// `--limit 400` a full clamped page of 50 comes back, and the old +/// `count == limit` test stayed silent — suppressing the warning in the +/// one case where the truncation is invisible. Hence the header. +/// +/// `total` is `X-Total-Count`, which Forgejo sends on this endpoint. The +/// *effective* page size isn't reported, but it doesn't need to be: a +/// short page is by definition the last one, so whenever more pages can +/// exist at all, `count` **is** the page size and `page * count` is how +/// many rows have been shown up to here. +/// +/// Deliberately not clamping `--limit` client-side to 50: that number is +/// the *remote's* configuration, and baking a peer's setting into the +/// client is how you get a value that's wrong on the one deployment that +/// changed it. +fn trailer(total: Option, count: u64, page: u64, limit: u64) -> Option { + // An empty page is the end, whatever else is true. + if count == 0 { + return None; + } + match total { + Some(total) => { + let shown = page.saturating_mul(count); + let remaining = total.saturating_sub(shown); + (remaining > 0).then(|| { + format!( + "… {shown} of {total} shown (page {page}, {count}/page) — {remaining} more; re-run with --page {}.", + page + 1 + ) + }) + } + // No header (older forge, or a proxy that dropped it): fall back + // to the full-page heuristic. It under-reports when the server + // clamped, which is why it is the fallback and not the rule. + None => (count == limit).then(|| { + format!( + "… {count} shown (page {page}); more results may exist — re-run with --page {}.", + page + 1 + ) + }), + } +} + /// Render one issue/PR as a single `#NNN [author] title` line. /// Defensive: missing fields drop to placeholders so a partial /// response from a future API change still produces readable output @@ -187,4 +250,45 @@ mod tests { ); assert_eq!(State::All.query_state(), IssueListIssuesQueryState::All); } + + #[test] + fn trailer_fires_on_a_clamped_page_the_old_heuristic_missed() { + // The regression this function exists for: `--limit 400` against + // a forge that clamps to 50, with 187 matches. `count != limit`, + // so the pre-header code printed nothing and the caller concluded + // the list was complete. + let msg = trailer(Some(187), 50, 1, 400).expect("must warn about the other 137"); + assert!(msg.contains("50 of 187"), "{msg}"); + assert!(msg.contains("137 more"), "{msg}"); + assert!(msg.contains("--page 2"), "{msg}"); + } + + #[test] + fn trailer_counts_from_the_served_page_size_not_the_requested_one() { + // Page 3 of a clamped 50/page walk has shown 150, not 3 * 400. + let msg = trailer(Some(187), 50, 3, 400).expect("187 > 150"); + assert!(msg.contains("150 of 187"), "{msg}"); + assert!(msg.contains("37 more"), "{msg}"); + } + + #[test] + fn trailer_silent_once_the_total_is_accounted_for() { + // Last page, short: 2 * 50 + ... -> 60 shown of 60. + assert_eq!(trailer(Some(60), 10, 6, 10), None); + // Exactly-full last page: no phantom "page 4". + assert_eq!(trailer(Some(150), 50, 3, 50), None); + // Empty page is the end even if the total disagrees (a filter + // the forge discarded, a concurrent close) — never advertise a + // next page we just proved is empty. + assert_eq!(trailer(Some(999), 0, 9, 30), None); + } + + #[test] + fn trailer_falls_back_to_the_full_page_heuristic_without_the_header() { + let msg = trailer(None, 30, 1, 30).expect("full page, unknown total"); + assert!(msg.contains("30 shown (page 1)"), "{msg}"); + assert!(msg.contains("--page 2"), "{msg}"); + // Short page with no header: the end, as far as we can tell. + assert_eq!(trailer(None, 12, 1, 30), None); + } }