hive-forge: cap timeline --limit so truncation detection can't go blind at the page-size boundary

hive-forge/src/verbs/timeline.rs

@@ -1,8 +1,8 @@
 //! `timeline <number> [--limit N]` — list timeline events on an
-//! issue or PR. Fills the gap where agents kept
+//! issue or PR. Fills the gap where agents kept falling back to curl
-//! falling back to curl for "who closed this?" / "when was this
+//! for "who closed this?" / "when was this labelled?" archaeology.
-//! labelled?" archaeology. Composes naturally with `view <n>` /
+//! Composes naturally with `view <n>` / `comments <n>` — separate verb
-//! `comments <n>` — separate verb keeps the existing shapes stable.
+//! 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`
+//! Default `--limit` is 10 (was 50), same silent-truncation-trap fix
-//! points agents at for "there's new activity, go look" — a big default
+//! as `comments`. **`more` is a boolean, not an exact count**: unlike
-//! was the same silent-truncation trap `comments`' old default was.
+//! `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
+//! `--limit` is capped at [`MAX_LIMIT`] (`PAGE_SIZE - 1`): the
-//! `more_before`/`more_after`. `comments` gets an exact count for free
+//! over-fetch-by-one trick needs `limit + 1` to fit inside Forgejo's
-//! from the issue's own `comments` field; the timeline endpoint has no
+//! hard 50-per-page cap, or the response silently clamps to 50 and the
-//! equivalent total (mixed event types, no `count` query), and
+//! truncation check can never fire even when there genuinely is more.
-//! `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;
@@ -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
+    /// Return the first `N` events, capped at 49 (see the module doc
-    /// the module doc comment.
+    /// 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!({