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!({