From c7d0e4c31743c5b427e7170e87d53100ccaf01cd Mon Sep 17 00:00:00 2001 From: damocles Date: Mon, 3 Aug 2026 19:01:30 +0200 Subject: [PATCH 1/3] hive-forge: smaller comments/timeline defaults, report truncation instead of hiding it --- hive-forge/src/verbs/comments.rs | 152 ++++++++++++++++++++++--------- hive-forge/src/verbs/timeline.rs | 45 ++++++--- 2 files changed, 144 insertions(+), 53 deletions(-) diff --git a/hive-forge/src/verbs/comments.rs b/hive-forge/src/verbs/comments.rs index 17bde9e6..39b48a58 100644 --- a/hive-forge/src/verbs/comments.rs +++ b/hive-forge/src/verbs/comments.rs @@ -1,30 +1,25 @@ //! `comments [--limit N | --tail N]` — list comments on an -//! issue or PR. Replaces the curl fallback; `--tail` -//! handles the paging-for-long-threads -//! awkwardness). +//! issue or PR. Replaces the curl fallback. //! -//! - `--limit N` (default 50, Forgejo's cap) returns the first N -//! comments — same shape this verb has always had. -//! - `--tail N` returns the *last* N comments in chronological -//! order. Reads the issue's `comments` count first to compute -//! which page contains the tail, then fetches only `ceil(N/50) + -//! 1` pages. No upstream cap — the work is bounded by `N`, not -//! by the thread's length, so it stays cheap even on threads with -//! thousands of comments. Use this for "what was the conclusion -//! on this long thread?" without scrolling through the whole -//! history. +//! - `--limit N` (default 10, was 50) returns the first N comments. A +//! big default was a silent trap on long threads: a plain `comments` +//! call reads as "I've read this thread" when it's really "I've read +//! the oldest N of it". +//! - `--tail N` returns the *last* N comments, chronological. Reads the +//! issue's `comments` count first to compute which page holds the +//! tail, then fetches only `ceil(N/50) + 1` pages — bounded by `N`, +//! not thread length, so it stays cheap on threads with thousands of +//! comments. +//! - **The count of comments outside whatever window is shown is +//! always reported** — never a silent truncation. //! -//! For PRs, review *bodies* (the summary text submitted with an -//! approve / request-changes / comment review) are merged in too: -//! they live in the `pulls//reviews` object, NOT the -//! issues/comments thread, so plain comment listings used to miss -//! them entirely and reviewers/authors silently lost feedback — -//! the gap this fix closes. They're always included regardless of -//! `--limit`/`--tail` -//! (reviews are few + high-signal) and tagged `[review: STATE]` so -//! they're distinguishable from issue-thread comments. +//! Review *bodies* on PRs are always merged in too (`pulls//reviews` +//! isn't the issues/comments thread, so a plain listing used to miss +//! them), tagged `[review: STATE]`. //! -//! Use the global `--json` flag for JSON output. +//! `--json` output is an object (`{"comments": [...], "more_before": N, +//! "more_after": N}`), not a bare array, so a script can read the +//! truncation counts too. use anyhow::Result; use clap::Args as ClapArgs; @@ -46,7 +41,7 @@ pub struct Args { pub(crate) number: u64, /// Number of comments from the start of the thread (max 50). /// Mutually exclusive with `--tail`. - #[arg(long, default_value_t = 50, conflicts_with = "tail")] + #[arg(long, default_value_t = 10, conflicts_with = "tail")] limit: u64, /// Return the last `N` comments (chronological). Mutually exclusive /// with `--limit`. @@ -56,9 +51,22 @@ pub struct Args { pub fn run(client: &Client, args: Args) -> Result<()> { let repo = client.repo(); - let thread = match args.tail { - Some(n) => fetch_tail(client, args.number, n)?, - None => fetch_head(client, args.number, args.limit)?, + let total = fetch_total(client, args.number)?; + // Truncation counts: how many comments sit outside the window we're + // about to show, on each side. A `--tail` window has nothing after it + // (it ends at the thread's current end); a `--limit`/default window + // has nothing before it (it starts at the thread's beginning). Derived + // from `thread.len()` rather than the requested `n`/`limit` so a + // shrunk-under-us thread (comments deleted mid-fetch) still reports + // accurately instead of the number we merely asked for. + let (thread, more_before, more_after) = if let Some(n) = args.tail { + let thread = fetch_tail(client, args.number, n, total)?; + let more_before = total.saturating_sub(thread.len()); + (thread, more_before, 0) + } else { + let thread = fetch_head(client, args.number, args.limit)?; + let more_after = total.saturating_sub(thread.len()); + (thread, 0, more_after) }; // Merge in PR review bodies (empty for issues — degrades to a // no-op) so review feedback isn't silently dropped. @@ -82,7 +90,11 @@ pub fn run(client: &Client, args: Args) -> Result<()> { }) }) .collect(); - print_json(&Value::Array(trimmed)) + print_json(&json!({ + "comments": trimmed, + "more_before": more_before, + "more_after": more_after, + })) } else { for c in &comments { let user = c @@ -100,10 +112,47 @@ pub fn run(client: &Client, args: Args) -> Result<()> { } println!(); } + if let Some(note) = truncation_note(more_before, more_after) { + println!("{note}"); + } Ok(()) } } +/// A one-line truncation note when the shown window doesn't cover the +/// whole thread — never a silent gap. `None` when both are zero (the +/// whole thread rode the wire). Returns a `String` rather than printing +/// directly so the three cases are unit-testable. +fn truncation_note(more_before: usize, more_after: usize) -> Option { + match (more_before > 0, more_after > 0) { + (false, false) => None, + (true, false) => Some(format!( + "({more_before} earlier comment(s) not shown — use --tail to see the start of the thread)" + )), + (false, true) => Some(format!( + "({more_after} more comment(s) not shown — use --tail N to see the latest instead)" + )), + (true, true) => Some(format!( + "({more_before} earlier + {more_after} later comment(s) not shown — use --tail N)" + )), + } +} + +/// Total comment count on an issue/PR, straight off the issue object. +/// Used both to plan `--tail`'s pagination and to report how many +/// comments sit outside whatever window ends up shown. +fn fetch_total(client: &Client, number: u64) -> Result { + let (owner, name) = client.owner_repo()?; + let issue = client + .api() + .issue_get_issue(owner, name, index(number)?) + .send()?; + Ok(issue + .comments + .and_then(|c| usize::try_from(c).ok()) + .unwrap_or(0)) +} + /// Serialize a typed comment page back to the JSON `Value` shape the /// merge + render pipeline works on (the structs serialize to the API /// wire shape, so downstream field access is unchanged). @@ -197,28 +246,22 @@ fn fetch_head(client: &Client, number: u64, limit: u64) -> Result> { } /// Fetch the last `n` comments on an issue/PR in chronological order. +/// `total` (the thread's comment count, from [`fetch_total`]) drives the +/// pagination plan and is the caller's, not fetched again here. /// /// Forgejo orders `/issues//comments` oldest-first and has no /// `direction=desc` knob, so naive "page everything and slice" pages /// from the WRONG end on long threads — the first 1000 comments -/// instead of the last `n`. Fix: read the issue's `comments` count -/// first to know how many exist, then start paginating from the -/// page that contains item `total - n`. Work is bounded by -/// `ceil(n/50) + 1` page fetches, regardless of thread length. -fn fetch_tail(client: &Client, number: u64, n: usize) -> Result> { - if n == 0 { +/// instead of the last `n`. Fix: use `total` to know how many exist, +/// then start paginating from the page that contains item +/// `total - n`. Work is bounded by `ceil(n/50) + 1` page fetches, +/// regardless of thread length. +fn fetch_tail(client: &Client, number: u64, n: usize, total: usize) -> Result> { + if n == 0 || total == 0 { return Ok(Vec::new()); } let (owner, name) = client.owner_repo()?; let idx = index(number)?; - let issue = client.api().issue_get_issue(owner, name, idx).send()?; - let total = issue - .comments - .and_then(|c| usize::try_from(c).ok()) - .unwrap_or(0); - if total == 0 { - return Ok(Vec::new()); - } // Cap `n` at the actual total so the math below stays in range // when the caller asks for more comments than exist. let n = n.min(total); @@ -330,6 +373,31 @@ mod tests { assert_eq!(bodies, vec!["c1", "r1", "c2"]); } + #[test] + fn truncation_note_silent_when_nothing_hidden() { + assert_eq!(truncation_note(0, 0), None); + } + + #[test] + fn truncation_note_after_only_points_at_tail() { + let note = truncation_note(0, 40).unwrap(); + assert!(note.contains("40"), "{note}"); + assert!(note.contains("--tail"), "{note}"); + } + + #[test] + fn truncation_note_before_only_mentions_earlier() { + let note = truncation_note(12, 0).unwrap(); + assert!(note.contains("12"), "{note}"); + assert!(note.contains("earlier"), "{note}"); + } + + #[test] + fn truncation_note_both_sides_mentions_both_counts() { + let note = truncation_note(3, 7).unwrap(); + assert!(note.contains('3') && note.contains('7'), "{note}"); + } + #[test] fn merge_with_no_reviews_is_identity() { // Issues have no reviews → fetch_review_bodies returns empty → diff --git a/hive-forge/src/verbs/timeline.rs b/hive-forge/src/verbs/timeline.rs index 2f976115..ffeff9d7 100644 --- a/hive-forge/src/verbs/timeline.rs +++ b/hive-forge/src/verbs/timeline.rs @@ -10,16 +10,27 @@ //! a human-readable form by default; pass the global `--json` flag //! for the raw API shape. //! -//! `--tail N` is a follow-up (the timeline endpoint doesn't expose a -//! total-count field so we can't use the count-then-page trick that -//! `comments --tail` uses; future shape probably mirrors -//! `comments --tail` once Forgejo grows a `count` query or we accept -//! the trailing-slice cost). +//! Default `--limit` is 10 (was 50): this is the verb `hive-forge-notify` +//! points agents at for "there's new activity, go look" — a big default +//! was the same silent-truncation trap `comments`' old default was. +//! +//! **`more` is a boolean, not an exact count**, unlike `comments`'s +//! `more_before`/`more_after`. `comments` gets an exact count for free +//! from the issue's own `comments` field; the timeline endpoint has no +//! equivalent total (mixed event types, no `count` query), and +//! `forgejo-api`'s typed `.send()` doesn't surface response headers +//! (checked: `X-Total-Count`, if Forgejo even sends one, is consumed +//! inside `FromResponse` and never reaches the caller — getting it would +//! mean bypassing the typed endpoint for a raw request, a bigger change +//! than this fix warrants). So this over-fetches by one: ask for +//! `limit + 1`, and if that many come back, trim to `limit` and report +//! "there's more" without saying how much. `--tail` stays a real +//! follow-up if an exact count/tail becomes worth the cost. use anyhow::Result; use clap::Args as ClapArgs; use forgejo_api::structs::IssueGetCommentsAndTimelineQuery; -use serde_json::Value; +use serde_json::{Value, json}; use crate::client::{Client, index}; use crate::verbs::print_json; @@ -28,13 +39,17 @@ use crate::verbs::print_json; pub struct Args { /// Issue or PR number. pub(crate) number: u64, - /// Page size (Forgejo caps at 50). Returns the first `N` events. - #[arg(long, default_value_t = 50)] + /// Return the first `N` events. Default kept small on purpose — see + /// the module doc comment. + #[arg(long, default_value_t = 10)] limit: u64, } pub fn run(client: &Client, args: Args) -> Result<()> { let (owner, name) = client.owner_repo()?; + // Over-fetch by one to detect truncation without an exact total — + // see the module doc comment for why there's no count query here. + let fetch_limit = args.limit.saturating_add(1); let (_, events) = client .api() .issue_get_comments_and_timeline( @@ -43,13 +58,16 @@ pub fn run(client: &Client, args: Args) -> Result<()> { index(args.number)?, IssueGetCommentsAndTimelineQuery::default(), ) - .page_size(u32::try_from(args.limit).unwrap_or(u32::MAX)) + .page_size(u32::try_from(fetch_limit).unwrap_or(u32::MAX)) .send()?; + let limit = usize::try_from(args.limit).unwrap_or(usize::MAX); + let more = events.len() > limit; + let events: Vec<_> = events.into_iter().take(limit).collect(); // Serialize back to the API's JSON shape so the per-type render // arms (and their tests) keep working on plain `Value`s. - let v = serde_json::to_value(events)?; + let v = serde_json::to_value(&events)?; if client.json_mode() { - return print_json(&v); + return print_json(&json!({ "events": v, "more": more })); } let Some(events) = v.as_array() else { return print_json(&v); @@ -57,6 +75,11 @@ pub fn run(client: &Client, args: Args) -> Result<()> { for ev in events { print_event(ev); } + if more { + println!( + "(more activity not shown — raise --limit to see it; exact count not available)" + ); + } Ok(()) } From f6629f0c231a263b4913a9a0d7a413e61a2b22d0 Mon Sep 17 00:00:00 2001 From: damocles Date: Mon, 3 Aug 2026 19:29:27 +0200 Subject: [PATCH 2/3] hive-forge: cap timeline --limit so truncation detection can't go blind at the page-size boundary --- hive-forge/src/verbs/timeline.rs | 88 ++++++++++++++++++++++---------- 1 file changed, 62 insertions(+), 26 deletions(-) diff --git a/hive-forge/src/verbs/timeline.rs b/hive-forge/src/verbs/timeline.rs index ffeff9d7..12e8674c 100644 --- a/hive-forge/src/verbs/timeline.rs +++ b/hive-forge/src/verbs/timeline.rs @@ -1,8 +1,8 @@ //! `timeline [--limit N]` — list timeline events on an -//! issue or PR. Fills the gap where agents kept -//! falling back to curl for "who closed this?" / "when was this -//! labelled?" archaeology. Composes naturally with `view ` / -//! `comments ` — separate verb keeps the existing shapes stable. +//! issue or PR. Fills the gap where agents kept falling back to curl +//! for "who closed this?" / "when was this labelled?" archaeology. +//! Composes naturally with `view ` / `comments ` — separate verb +//! keeps the existing shapes stable. //! //! Forgejo's `/issues/{n}/timeline` endpoint returns BOTH the actual //! comments AND the event entries (label, assignee, close, reopen, @@ -10,22 +10,18 @@ //! a human-readable form by default; pass the global `--json` flag //! for the raw API shape. //! -//! Default `--limit` is 10 (was 50): this is the verb `hive-forge-notify` -//! points agents at for "there's new activity, go look" — a big default -//! was the same silent-truncation trap `comments`' old default was. +//! Default `--limit` is 10 (was 50), same silent-truncation-trap fix +//! as `comments`. **`more` is a boolean, not an exact count**: unlike +//! `comments` (which reads the issue's own `comments` field), +//! the timeline endpoint has no total, and `forgejo-api`'s typed +//! `.send()` never surfaces response headers to check `X-Total-Count` +//! against — so this over-fetches by one (`limit + 1`) and reports +//! "there's more" without saying how much. //! -//! **`more` is a boolean, not an exact count**, unlike `comments`'s -//! `more_before`/`more_after`. `comments` gets an exact count for free -//! from the issue's own `comments` field; the timeline endpoint has no -//! equivalent total (mixed event types, no `count` query), and -//! `forgejo-api`'s typed `.send()` doesn't surface response headers -//! (checked: `X-Total-Count`, if Forgejo even sends one, is consumed -//! inside `FromResponse` and never reaches the caller — getting it would -//! mean bypassing the typed endpoint for a raw request, a bigger change -//! than this fix warrants). So this over-fetches by one: ask for -//! `limit + 1`, and if that many come back, trim to `limit` and report -//! "there's more" without saying how much. `--tail` stays a real -//! follow-up if an exact count/tail becomes worth the cost. +//! `--limit` is capped at [`MAX_LIMIT`] (`PAGE_SIZE - 1`): the +//! over-fetch-by-one trick needs `limit + 1` to fit inside Forgejo's +//! hard 50-per-page cap, or the response silently clamps to 50 and the +//! truncation check can never fire even when there genuinely is more. use anyhow::Result; use clap::Args as ClapArgs; @@ -35,21 +31,43 @@ use serde_json::{Value, json}; use crate::client::{Client, index}; use crate::verbs::print_json; +/// Forgejo's per-page cap on the timeline endpoint — same hard ceiling +/// `comments.rs`'s `PAGE_SIZE` documents for `/comments`. +const PAGE_SIZE: u64 = 50; + +/// Largest `--limit` the over-fetch-by-one trick can still detect +/// truncation at: `limit + 1` must stay within [`PAGE_SIZE`], or the +/// response silently clamps to `PAGE_SIZE` and `more` reads `false` even +/// when there's genuinely more. +const MAX_LIMIT: u64 = PAGE_SIZE - 1; + #[derive(ClapArgs)] pub struct Args { /// Issue or PR number. pub(crate) number: u64, - /// Return the first `N` events. Default kept small on purpose — see - /// the module doc comment. + /// Return the first `N` events, capped at 49 (see the module doc + /// comment for why). Default kept small on purpose. #[arg(long, default_value_t = 10)] limit: u64, } +/// Effective `--limit` plus whether the request was clamped to +/// [`MAX_LIMIT`]. Pure so the boundary math is unit-testable without a +/// network call. +fn clamp_limit(requested: u64) -> (u64, bool) { + let limit = requested.min(MAX_LIMIT); + (limit, limit < requested) +} + pub fn run(client: &Client, args: Args) -> Result<()> { let (owner, name) = client.owner_repo()?; + // Clamp before the over-fetch-by-one math below: `limit + 1` has to + // stay within Forgejo's per-page cap or the truncation check goes + // silently blind right at the boundary — see MAX_LIMIT's doc comment. + let (limit, clamped) = clamp_limit(args.limit); // Over-fetch by one to detect truncation without an exact total — // see the module doc comment for why there's no count query here. - let fetch_limit = args.limit.saturating_add(1); + let fetch_limit = limit.saturating_add(1); let (_, events) = client .api() .issue_get_comments_and_timeline( @@ -60,14 +78,14 @@ pub fn run(client: &Client, args: Args) -> Result<()> { ) .page_size(u32::try_from(fetch_limit).unwrap_or(u32::MAX)) .send()?; - let limit = usize::try_from(args.limit).unwrap_or(usize::MAX); + let limit = usize::try_from(limit).unwrap_or(usize::MAX); let more = events.len() > limit; let events: Vec<_> = events.into_iter().take(limit).collect(); // Serialize back to the API's JSON shape so the per-type render // arms (and their tests) keep working on plain `Value`s. let v = serde_json::to_value(&events)?; if client.json_mode() { - return print_json(&json!({ "events": v, "more": more })); + return print_json(&json!({ "events": v, "more": more, "limit_clamped": clamped })); } let Some(events) = v.as_array() else { return print_json(&v); @@ -75,11 +93,15 @@ pub fn run(client: &Client, args: Args) -> Result<()> { for ev in events { print_event(ev); } - if more { + if clamped { println!( - "(more activity not shown — raise --limit to see it; exact count not available)" + "(--limit {} is above the {MAX_LIMIT} cap this verb can reliably detect truncation at — clamped)", + args.limit ); } + if more { + println!("(more activity not shown — raise --limit to see it; exact count not available)"); + } Ok(()) } @@ -266,6 +288,20 @@ mod tests { //! duplicating the logic" — addressed. use super::*; + #[test] + fn clamp_limit_passes_small_requests_through() { + assert_eq!(clamp_limit(10), (10, false)); + assert_eq!(clamp_limit(MAX_LIMIT), (MAX_LIMIT, false)); + } + + #[test] + fn clamp_limit_caps_requests_above_the_boundary() { + // Regression: `limit + 1` must never exceed Forgejo's PAGE_SIZE, + // or the over-fetch-by-one truncation check goes silently blind. + assert_eq!(clamp_limit(PAGE_SIZE), (MAX_LIMIT, true)); + assert_eq!(clamp_limit(1000), (MAX_LIMIT, true)); + } + #[test] fn comment_renders_body_inline() { let ev = serde_json::json!({ From 4cf647aa7081d97e254db9cd0351e686da1e336e Mon Sep 17 00:00:00 2001 From: damocles Date: Mon, 3 Aug 2026 20:39:39 +0200 Subject: [PATCH 3/3] hive-forge: add --since cursor paging to comments and timeline --- hive-forge/src/verbs/comments.rs | 120 +++++++++++++++++++++++-------- hive-forge/src/verbs/mod.rs | 50 +++++++++++++ hive-forge/src/verbs/timeline.rs | 59 +++++---------- 3 files changed, 159 insertions(+), 70 deletions(-) diff --git a/hive-forge/src/verbs/comments.rs b/hive-forge/src/verbs/comments.rs index 39b48a58..585a1582 100644 --- a/hive-forge/src/verbs/comments.rs +++ b/hive-forge/src/verbs/comments.rs @@ -12,62 +12,81 @@ //! comments. //! - **The count of comments outside whatever window is shown is //! always reported** — never a silent truncation. +//! - `--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 to fetch only +//! what's new. Mutually exclusive with `--tail`. No total exists for +//! a since-filtered query, so this uses the same over-fetch-by-one +//! trick `timeline`'s `--limit` does, and `--limit` is clamped the +//! same way (see `crate::verbs::MAX_LIMIT`). //! //! Review *bodies* on PRs are always merged in too (`pulls//reviews` //! isn't the issues/comments thread, so a plain listing used to miss //! them), tagged `[review: STATE]`. //! //! `--json` output is an object (`{"comments": [...], "more_before": N, -//! "more_after": N}`), not a bare array, so a script can read the -//! truncation counts too. +//! "more_after": N, "since_more": bool}`), not a bare array, so a +//! script can read the truncation info too. use anyhow::Result; use clap::Args as ClapArgs; use forgejo_api::structs::IssueGetCommentsQuery; use serde_json::{Value, json}; +use time::OffsetDateTime; use crate::client::{Client, index}; use crate::notify; -use crate::verbs::{print_json, rfc3339}; - -/// Forgejo's per-page comment cap. The API caps `limit` at 50 even -/// if a higher value is requested; pin it explicitly so the math -/// downstream doesn't depend on a hidden default. -const PAGE_SIZE: usize = 50; +use crate::verbs::{MAX_LIMIT, PAGE_SIZE, clamp_limit, parse_rfc3339, print_json, rfc3339}; #[derive(ClapArgs)] pub struct Args { /// Issue or PR number. pub(crate) number: u64, - /// Number of comments from the start of the thread (max 50). - /// Mutually exclusive with `--tail`. + /// Number of comments from the start of the thread, or (with + /// `--since`) the most this call returns — capped at + /// [`crate::verbs::MAX_LIMIT`] in the latter case. Mutually + /// exclusive with `--tail`. #[arg(long, default_value_t = 10, conflicts_with = "tail")] limit: u64, /// Return the last `N` comments (chronological). Mutually exclusive - /// with `--limit`. - #[arg(long)] + /// with `--limit`/`--since`. + #[arg(long, conflicts_with = "since")] tail: Option, + /// Only show comments at or after this RFC3339 timestamp (same + /// format this verb's own output prints). Mutually exclusive with + /// `--tail`. + #[arg(long)] + since: Option, } pub fn run(client: &Client, args: Args) -> Result<()> { let repo = client.repo(); - let total = fetch_total(client, args.number)?; - // Truncation counts: how many comments sit outside the window we're - // about to show, on each side. A `--tail` window has nothing after it - // (it ends at the thread's current end); a `--limit`/default window - // has nothing before it (it starts at the thread's beginning). Derived - // from `thread.len()` rather than the requested `n`/`limit` so a + // Truncation info: what sits outside the window we're about to show. + // A `--tail` window has nothing after it (it ends at the thread's + // current end); a `--limit`/default window has nothing before it (it + // 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. `more_before`/`more_after` are derived from + // `thread.len()` rather than the requested `n`/`limit` so a // shrunk-under-us thread (comments deleted mid-fetch) still reports // accurately instead of the number we merely asked for. - let (thread, more_before, more_after) = if let Some(n) = args.tail { - let thread = fetch_tail(client, args.number, n, total)?; - let more_before = total.saturating_sub(thread.len()); - (thread, more_before, 0) - } else { - let thread = fetch_head(client, args.number, args.limit)?; - let more_after = total.saturating_sub(thread.len()); - (thread, 0, more_after) - }; + 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); + let (thread, more) = fetch_since(client, args.number, since, limit)?; + (thread, 0, 0, more, clamped) + } else if let Some(n) = args.tail { + let total = fetch_total(client, args.number)?; + let thread = fetch_tail(client, args.number, n, total)?; + let more_before = total.saturating_sub(thread.len()); + (thread, more_before, 0, false, false) + } else { + let total = fetch_total(client, args.number)?; + let thread = fetch_head(client, args.number, args.limit)?; + let more_after = total.saturating_sub(thread.len()); + (thread, 0, more_after, false, false) + }; // Merge in PR review bodies (empty for issues — degrades to a // no-op) so review feedback isn't silently dropped. let comments = merge_chronological(thread, fetch_review_bodies(client, args.number)); @@ -94,6 +113,8 @@ pub fn run(client: &Client, args: Args) -> Result<()> { "comments": trimmed, "more_before": more_before, "more_after": more_after, + "since_more": since_more, + "since_limit_clamped": since_clamped, })) } else { for c in &comments { @@ -112,6 +133,17 @@ 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 + ); + } + if since_more { + println!( + "(more comments since this timestamp not shown — raise --limit or bump --since; exact count not available)" + ); + } if let Some(note) = truncation_note(more_before, more_after) { println!("{note}"); } @@ -265,7 +297,7 @@ fn fetch_tail(client: &Client, number: u64, n: usize, total: usize) -> Result Result Result<(Vec, bool)> { + let (owner, name) = client.owner_repo()?; + let query = IssueGetCommentsQuery { + 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)) +} + #[cfg(test)] mod tests { use super::*; @@ -305,7 +367,7 @@ mod tests { /// easy to off-by-one — without touching the network. fn tail_plan(total: usize, n: usize) -> (usize, usize) { let n = n.min(total); - let page_size = PAGE_SIZE; + let page_size = usize::try_from(PAGE_SIZE).unwrap_or(usize::MAX); let start_idx = total - n; let start_page = (start_idx / page_size) + 1; let last_page = (total - 1) / page_size + 1; diff --git a/hive-forge/src/verbs/mod.rs b/hive-forge/src/verbs/mod.rs index f2cd9bc8..e86ed8ab 100644 --- a/hive-forge/src/verbs/mod.rs +++ b/hive-forge/src/verbs/mod.rs @@ -69,6 +69,56 @@ pub(crate) fn rfc3339(ts: Option) -> Option { ts.and_then(|t| t.format(&Rfc3339).ok()) } +/// Parse a `--since`/`--before` CLI argument as RFC 3339 — the inverse of +/// [`rfc3339`], so a value copied straight from this tool's own output +/// (every row prints its `created_at` in this exact shape) round-trips +/// without reformatting. A bad value gets a message naming what was +/// typed, not a bare parser error. +pub(crate) fn parse_rfc3339(s: &str) -> Result { + OffsetDateTime::parse(s, &Rfc3339) + .map_err(|e| anyhow::anyhow!("`{s}` isn't a valid RFC 3339 timestamp: {e}")) +} + +/// 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 +/// 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; + +/// The highest `--limit` an over-fetch-by-one truncation check +/// (`fetch_limit = limit + 1`) can still detect: `PAGE_SIZE - 1`. At +/// `limit == PAGE_SIZE` the `+1` request silently clamps to `PAGE_SIZE` +/// server-side and the truncation check goes blind exactly when there's +/// the most data to miss. +pub(crate) const MAX_LIMIT: u64 = PAGE_SIZE - 1; + +/// Cap `requested` at [`MAX_LIMIT`], reporting whether it had to. Pure so +/// the boundary math is unit-testable without a network call. +pub(crate) fn clamp_limit(requested: u64) -> (u64, bool) { + let limit = requested.min(MAX_LIMIT); + (limit, limit < requested) +} + +#[cfg(test)] +mod page_limit_tests { + use super::{MAX_LIMIT, PAGE_SIZE, clamp_limit}; + + #[test] + fn clamp_limit_passes_small_requests_through() { + assert_eq!(clamp_limit(10), (10, false)); + assert_eq!(clamp_limit(MAX_LIMIT), (MAX_LIMIT, false)); + } + + #[test] + fn clamp_limit_caps_requests_above_the_boundary() { + // Regression: `limit + 1` must never exceed Forgejo's PAGE_SIZE, + // or the over-fetch-by-one truncation check goes silently blind. + assert_eq!(clamp_limit(PAGE_SIZE), (MAX_LIMIT, true)); + assert_eq!(clamp_limit(1000), (MAX_LIMIT, true)); + } +} + /// Issue-vs-PR kind, for the `pr ` / `issue ` sub-command /// validation. #[derive(Clone, Copy)] diff --git a/hive-forge/src/verbs/timeline.rs b/hive-forge/src/verbs/timeline.rs index 12e8674c..cc8dc09e 100644 --- a/hive-forge/src/verbs/timeline.rs +++ b/hive-forge/src/verbs/timeline.rs @@ -18,10 +18,14 @@ //! against — so this over-fetches by one (`limit + 1`) and reports //! "there's more" without saying how much. //! -//! `--limit` is capped at [`MAX_LIMIT`] (`PAGE_SIZE - 1`): the +//! `--limit` is capped at [`crate::verbs::MAX_LIMIT`]: the //! over-fetch-by-one trick needs `limit + 1` to fit inside Forgejo's -//! hard 50-per-page cap, or the response silently clamps to 50 and the +//! hard per-page cap, or the response silently clamps and the //! truncation check can never fire even when there genuinely is more. +//! +//! `--since ` filters to events at or after that timestamp — +//! a cursor: feed the last-seen row's own `created_at` back in next +//! time instead of re-running with a higher `--limit`. use anyhow::Result; use clap::Args as ClapArgs; @@ -29,17 +33,7 @@ use forgejo_api::structs::IssueGetCommentsAndTimelineQuery; use serde_json::{Value, json}; use crate::client::{Client, index}; -use crate::verbs::print_json; - -/// Forgejo's per-page cap on the timeline endpoint — same hard ceiling -/// `comments.rs`'s `PAGE_SIZE` documents for `/comments`. -const PAGE_SIZE: u64 = 50; - -/// Largest `--limit` the over-fetch-by-one trick can still detect -/// truncation at: `limit + 1` must stay within [`PAGE_SIZE`], or the -/// response silently clamps to `PAGE_SIZE` and `more` reads `false` even -/// when there's genuinely more. -const MAX_LIMIT: u64 = PAGE_SIZE - 1; +use crate::verbs::{MAX_LIMIT, clamp_limit, parse_rfc3339, print_json}; #[derive(ClapArgs)] pub struct Args { @@ -49,14 +43,11 @@ pub struct Args { /// comment for why). Default kept small on purpose. #[arg(long, default_value_t = 10)] limit: u64, -} - -/// Effective `--limit` plus whether the request was clamped to -/// [`MAX_LIMIT`]. Pure so the boundary math is unit-testable without a -/// network call. -fn clamp_limit(requested: u64) -> (u64, bool) { - let limit = requested.min(MAX_LIMIT); - (limit, limit < requested) + /// Only show events at or after this RFC3339 timestamp (same format + /// this verb's own output prints) — pass back the last-seen row's + /// `created_at` to fetch only what's new. + #[arg(long)] + since: Option, } pub fn run(client: &Client, args: Args) -> Result<()> { @@ -65,17 +56,17 @@ pub fn run(client: &Client, args: Args) -> Result<()> { // stay within Forgejo's per-page cap or the truncation check goes // silently blind right at the boundary — see MAX_LIMIT's doc comment. let (limit, clamped) = clamp_limit(args.limit); + let since = args.since.as_deref().map(parse_rfc3339).transpose()?; + let query = IssueGetCommentsAndTimelineQuery { + since, + before: None, + }; // Over-fetch by one to detect truncation without an exact total — // see the module doc comment for why there's no count query here. let fetch_limit = limit.saturating_add(1); let (_, events) = client .api() - .issue_get_comments_and_timeline( - owner, - name, - index(args.number)?, - IssueGetCommentsAndTimelineQuery::default(), - ) + .issue_get_comments_and_timeline(owner, name, index(args.number)?, query) .page_size(u32::try_from(fetch_limit).unwrap_or(u32::MAX)) .send()?; let limit = usize::try_from(limit).unwrap_or(usize::MAX); @@ -288,20 +279,6 @@ mod tests { //! duplicating the logic" — addressed. use super::*; - #[test] - fn clamp_limit_passes_small_requests_through() { - assert_eq!(clamp_limit(10), (10, false)); - assert_eq!(clamp_limit(MAX_LIMIT), (MAX_LIMIT, false)); - } - - #[test] - fn clamp_limit_caps_requests_above_the_boundary() { - // Regression: `limit + 1` must never exceed Forgejo's PAGE_SIZE, - // or the over-fetch-by-one truncation check goes silently blind. - assert_eq!(clamp_limit(PAGE_SIZE), (MAX_LIMIT, true)); - assert_eq!(clamp_limit(1000), (MAX_LIMIT, true)); - } - #[test] fn comment_renders_body_inline() { let ev = serde_json::json!({