From 3d81266483ce6ae420fe0539bbe5bc1c98e06245 Mon Sep 17 00:00:00 2001 From: atlas Date: Fri, 9 Oct 2026 21:44:00 +0200 Subject: [PATCH] hive-forge: comments --since --limit returns N, not at most 49 The issue-comments endpoint takes no page or limit, so `comments --since` cut its window client-side from a whole-list response. The leftover over-fetch-by-one request (`.page_size(limit + 1)`) was a no-op and the `--limit` clamp to MAX_LIMIT (49) guarded a truncation check that no longer needs one. Drop both: `--since T --limit N` now returns N comments. `comments --json` still emits `since_limit_clamped`, always `false`. `timeline` is untouched; its endpoint pages and its clamp stays. --- docs/tools/forge-cli.md | 4 +- hive-forge/src/verbs/comments.rs | 78 ++++++++++---------------- hive-forge/src/verbs/mod.rs | 2 +- hive-forge/tests/comments_no_paging.rs | 20 +++++++ 4 files changed, 54 insertions(+), 50 deletions(-) diff --git a/docs/tools/forge-cli.md b/docs/tools/forge-cli.md index 8c9e5a31..85e652b9 100644 --- a/docs/tools/forge-cli.md +++ b/docs/tools/forge-cli.md @@ -242,7 +242,7 @@ List comments on the issue ###### **Options:** -* `--limit ` — Number of comments from the start of the thread (oldest-first), or (with `--since`) the most this call returns — capped at [`crate::verbs::MAX_LIMIT`] in the latter case. An explicit opt-in: with neither this nor `--tail` given, the default is the newest 10 (see `--tail`). Mutually exclusive with `--tail` +* `--limit ` — Number of comments from the start of the thread (oldest-first), or (with `--since`) the most this call returns. An explicit opt-in: with neither this nor `--tail` given, the default is the newest 10 (see `--tail`). Mutually exclusive with `--tail` * `--tail ` — Return the last `N` comments (chronological) — the most recent activity. Defaults to `N` = 10 when you pass neither `--limit` nor `--tail`. Mutually exclusive with `--limit`/`--since` * `--since ` — Only show comments at or after this RFC3339 timestamp (same format this verb's own output prints). Mutually exclusive with `--tail` * `--show-reactions` — Fetch + display each shown comment's reaction summary. Costs one extra request per comment shown — opt-in, not the default @@ -704,7 +704,7 @@ List comments on the PR ###### **Options:** -* `--limit ` — Number of comments from the start of the thread (oldest-first), or (with `--since`) the most this call returns — capped at [`crate::verbs::MAX_LIMIT`] in the latter case. An explicit opt-in: with neither this nor `--tail` given, the default is the newest 10 (see `--tail`). Mutually exclusive with `--tail` +* `--limit ` — Number of comments from the start of the thread (oldest-first), or (with `--since`) the most this call returns. An explicit opt-in: with neither this nor `--tail` given, the default is the newest 10 (see `--tail`). Mutually exclusive with `--tail` * `--tail ` — Return the last `N` comments (chronological) — the most recent activity. Defaults to `N` = 10 when you pass neither `--limit` nor `--tail`. Mutually exclusive with `--limit`/`--since` * `--since ` — Only show comments at or after this RFC3339 timestamp (same format this verb's own output prints). Mutually exclusive with `--tail` * `--show-reactions` — Fetch + display each shown comment's reaction summary. Costs one extra request per comment shown — opt-in, not the default diff --git a/hive-forge/src/verbs/comments.rs b/hive-forge/src/verbs/comments.rs index b6101448..dc9784c0 100644 --- a/hive-forge/src/verbs/comments.rs +++ b/hive-forge/src/verbs/comments.rs @@ -16,17 +16,18 @@ //! - `--since ` filters to comments at or after that //! timestamp instead of a head/tail window — a cursor: feed the //! last-seen row's own `created_at` back in next time. Mutually -//! exclusive with `--tail`; `--limit` clamps like `timeline`'s own -//! does (see `crate::verbs::MAX_LIMIT`). +//! exclusive with `--tail`; `--limit` windows the filtered list as +//! given, uncapped. //! //! Review *bodies* on PRs are always merged in too (`pulls//reviews` //! isn't the issues/comments thread), tagged `[review: STATE]`, with a //! `(N line comment(s) — see 'pr reviews')` pointer for inline comments. //! //! `--json` output is an object (`{"comments": [...], "more_before": N, -//! "more_after": N, "since_more": bool}`), not a bare array, so a -//! script can read the truncation info too. `--show-reactions` adds a -//! per-comment reaction summary — see that flag's own doc for the cost. +//! "more_after": N, "since_more": bool, "since_limit_clamped": false}`), +//! not a bare array, so a script can read the truncation info too. +//! `--show-reactions` adds a per-comment reaction summary — see that +//! flag's own doc for the cost. use anyhow::Result; use clap::Args as ClapArgs; @@ -37,8 +38,7 @@ use time::OffsetDateTime; use crate::client::{Client, index}; use crate::notify; use crate::verbs::{ - MAX_LIMIT, all_pages, clamp_limit, comment_reactions, parse_rfc3339, print_json, - reaction_summary, rfc3339, + all_pages, comment_reactions, parse_rfc3339, print_json, reaction_summary, rfc3339, }; #[derive(ClapArgs)] @@ -46,8 +46,7 @@ pub struct Args { /// Issue or PR number. pub(crate) number: u64, /// Number of comments from the start of the thread (oldest-first), - /// or (with `--since`) the most this call returns — capped at - /// [`crate::verbs::MAX_LIMIT`] in the latter case. An explicit + /// or (with `--since`) the most this call returns. An explicit /// opt-in: with neither this nor `--tail` given, the default is the /// newest 10 (see `--tail`). Mutually exclusive with `--tail`. #[arg(long, conflicts_with = "tail")] @@ -76,24 +75,22 @@ pub fn run(client: &Client, args: Args) -> Result<()> { // starts at the thread's beginning); a `--since` window is a boolean // "there's more" (no total exists for a since-filtered query) rather // than an exact count. - let (thread, more_before, more_after, since_more, since_clamped) = - if let Some(since_str) = &args.since { - let since = parse_rfc3339(since_str)?; - let (limit, clamped) = clamp_limit(args.limit.unwrap_or(10)); - let (thread, more) = fetch_since(client, args.number, since, limit)?; - (thread, 0, 0, more, clamped) - } else if let Some(limit) = args.limit { - let (thread, more_after) = head(fetch_thread(client, args.number)?, limit); - (thread, 0, more_after, false, false) - } else { - // Default (no `--limit`/`--tail`/`--since`): same window as - // an explicit `--tail 10` — the newest activity, not the - // oldest. `args.tail` still wins if the agent passed a - // different count explicitly. - let n = args.tail.unwrap_or(10); - let (thread, more_before) = tail(fetch_thread(client, args.number)?, n); - (thread, more_before, 0, false, false) - }; + let (thread, more_before, more_after, since_more) = if let Some(since_str) = &args.since { + let since = parse_rfc3339(since_str)?; + let (thread, more) = fetch_since(client, args.number, since, args.limit.unwrap_or(10))?; + (thread, 0, 0, more) + } else if let Some(limit) = args.limit { + let (thread, more_after) = head(fetch_thread(client, args.number)?, limit); + (thread, 0, more_after, false) + } else { + // Default (no `--limit`/`--tail`/`--since`): same window as + // an explicit `--tail 10` — the newest activity, not the + // oldest. `args.tail` still wins if the agent passed a + // different count explicitly. + let n = args.tail.unwrap_or(10); + let (thread, more_before) = tail(fetch_thread(client, args.number)?, n); + (thread, more_before, 0, false) + }; // Merge in PR review bodies (empty for issues — degrades to a // no-op) so review feedback isn't silently dropped. let mut comments = merge_chronological(thread, fetch_review_bodies(client, args.number)); @@ -126,7 +123,7 @@ pub fn run(client: &Client, args: Args) -> Result<()> { "more_before": more_before, "more_after": more_after, "since_more": since_more, - "since_limit_clamped": since_clamped, + "since_limit_clamped": false, })) } else { for c in &comments { @@ -163,12 +160,6 @@ pub fn run(client: &Client, args: Args) -> Result<()> { } println!(); } - if since_clamped { - println!( - "(--limit {} is above the {MAX_LIMIT} cap --since can reliably detect truncation at — clamped)", - args.limit.unwrap_or(10) - ); - } if since_more { println!( "(more comments since this timestamp not shown — raise --limit or bump --since; exact count not available)" @@ -388,12 +379,10 @@ fn tail(mut thread: Vec, n: usize) -> (Vec, usize) { (thread, more_before) } -/// Fetch comments at or after `since`, capped at `limit` (already -/// clamped to [`crate::verbs::MAX_LIMIT`] by the caller). No total exists -/// for a since-filtered query — Forgejo's API reports none — so this -/// uses the same over-fetch-by-one trick `timeline`'s `--limit` does: -/// ask for `limit + 1`, and a full extra row means there's more. Returns -/// `(comments, more)`, `more` a boolean rather than an exact count. +/// Comments at or after `since`, the first `limit` of them. The endpoint +/// takes no page or limit, so the filtered list arrives whole and `more` +/// (whether any follow the window) is read off its length. A boolean +/// rather than a count: a since-filtered window has no total to report. fn fetch_since( client: &Client, number: u64, @@ -405,17 +394,12 @@ fn fetch_since( since: Some(since), ..Default::default() }; - let fetch_limit = limit.saturating_add(1); let (_, comments) = client .api() .issue_get_comments(owner, name, index(number)?, query) - .page_size(u32::try_from(fetch_limit).unwrap_or(u32::MAX)) .send()?; - let mut values = to_values(comments)?; - let limit = usize::try_from(limit).unwrap_or(usize::MAX); - let more = values.len() > limit; - values.truncate(limit); - Ok((values, more)) + let (values, more_after) = head(to_values(comments)?, limit); + Ok((values, more_after > 0)) } #[cfg(test)] diff --git a/hive-forge/src/verbs/mod.rs b/hive-forge/src/verbs/mod.rs index 8acd1645..ae80ef5e 100644 --- a/hive-forge/src/verbs/mod.rs +++ b/hive-forge/src/verbs/mod.rs @@ -101,7 +101,7 @@ pub(crate) fn parse_rfc3339(s: &str) -> Result { /// Forgejo's per-page cap, shared by every listing verb that over-fetches /// by one to detect truncation without an exact total (`timeline`'s -/// `--limit`, `comments`' `--since`). The API silently clamps a requested +/// `--limit`). The API silently clamps a requested /// page size to this value, so it's pinned explicitly rather than left as /// a hidden default downstream math could drift out of sync with. pub(crate) const PAGE_SIZE: u64 = 50; diff --git a/hive-forge/tests/comments_no_paging.rs b/hive-forge/tests/comments_no_paging.rs index 95594ad2..2b7999af 100644 --- a/hive-forge/tests/comments_no_paging.rs +++ b/hive-forge/tests/comments_no_paging.rs @@ -137,3 +137,23 @@ fn comments_tail_returns_the_last_n() { assert!(!stdout.contains(&format!("comment {}", THREAD_LEN - 4))); assert!(stdout.contains(&format!("({} earlier comment(s)", THREAD_LEN - 3))); } + +#[test] +fn comments_since_limit_above_a_page_returns_exactly_limit() { + let stdout = hive_forge( + "since-limit", + &[ + "--json", + "comments", + NUMBER, + "--since", + "2026-10-09T00:00:00Z", + "--limit", + "55", + ], + ); + let out: serde_json::Value = serde_json::from_str(&stdout).expect("comments --json output"); + assert_eq!(out["comments"].as_array().map(Vec::len), Some(55)); + assert_eq!(out["since_more"], true); + assert_eq!(out["since_limit_clamped"], false); +}