diff --git a/hive-forge/src/verbs/comments.rs b/hive-forge/src/verbs/comments.rs index 65678aeb..b2095675 100644 --- a/hive-forge/src/verbs/comments.rs +++ b/hive-forge/src/verbs/comments.rs @@ -1,15 +1,15 @@ //! `comments [--limit N | --tail N]` — list comments on an //! issue or PR. Replaces the curl fallback. //! -//! - `--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. +//! - `--limit N` returns the first N comments (oldest-first) — an +//! explicit opt-in: a plain `comments` call used to read as "I've read +//! this thread" when it was really "the oldest N of it", so the +//! default moved to `--tail`'s window instead (see below). +//! - `--tail N` returns the *last* N comments, chronological — and with +//! neither flag given, THIS is the default (N = 10): most-recent +//! activity. 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. //! - **The count of comments outside whatever window is shown is //! always reported** — never a silent truncation. //! - `--since ` filters to comments at or after that @@ -45,14 +45,16 @@ use crate::verbs::{ pub struct Args { /// Issue or PR number. pub(crate) number: u64, - /// 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`/`--since`. + /// 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`. + #[arg(long, conflicts_with = "tail")] + limit: Option, + /// Return the last `N` comments (chronological) — the most recent + /// activity. This is the default (`N` = 10) when neither `--limit` + /// nor `--tail` is given. Mutually exclusive with `--limit`/`--since`. #[arg(long, conflicts_with = "since")] tail: Option, /// Only show comments at or after this RFC3339 timestamp (same @@ -69,8 +71,8 @@ pub struct Args { pub fn run(client: &Client, args: Args) -> Result<()> { let repo = client.repo()?; // 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 + // A `--tail`/default window has nothing after it (it ends at the + // thread's current end); a `--limit` 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 @@ -80,19 +82,24 @@ pub fn run(client: &Client, args: Args) -> Result<()> { 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 (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(n) = args.tail { + } else if let Some(limit) = args.limit { + let total = fetch_total(client, args.number)?; + let thread = fetch_head(client, args.number, limit)?; + let more_after = total.saturating_sub(thread.len()); + (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 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. @@ -166,7 +173,7 @@ pub fn run(client: &Client, args: Args) -> Result<()> { if since_clamped { println!( "(--limit {} is above the {MAX_LIMIT} cap --since can reliably detect truncation at — clamped)", - args.limit + args.limit.unwrap_or(10) ); } if since_more { @@ -189,7 +196,7 @@ 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)" + "({more_before} earlier comment(s) not shown — use --limit 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)" @@ -554,10 +561,15 @@ mod tests { } #[test] - fn truncation_note_before_only_mentions_earlier() { + fn truncation_note_before_only_points_at_limit() { + // Regression: this used to (wrongly) suggest `--tail` — the flag + // that produced the very window you're already looking at when + // more_before is nonzero. `--limit` is what actually reaches the + // start of the thread. let note = truncation_note(12, 0).unwrap(); assert!(note.contains("12"), "{note}"); assert!(note.contains("earlier"), "{note}"); + assert!(note.contains("--limit"), "{note}"); } #[test] diff --git a/hive-forge/src/verbs/timeline.rs b/hive-forge/src/verbs/timeline.rs index cc8dc09e..350f7e55 100644 --- a/hive-forge/src/verbs/timeline.rs +++ b/hive-forge/src/verbs/timeline.rs @@ -1,48 +1,54 @@ -//! `timeline [--limit N]` — list timeline events on an -//! issue or PR. Fills the gap where agents kept falling back to curl +//! `timeline [--limit N | --tail N]` — list timeline events +//! (comments + activity: labels, assignees, close/reopen, pushes, …) 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. //! -//! Forgejo's `/issues/{n}/timeline` endpoint returns BOTH the actual -//! comments AND the event entries (label, assignee, close, reopen, -//! `pull_push`, etc.) in chronological order. We render each row in -//! a human-readable form by default; pass the global `--json` flag -//! for the raw API shape. +//! Same three windows as `comments`, same reasoning: `--limit N` returns +//! the first N events (oldest-first) — an explicit opt-in now, since a +//! plain `timeline` call reading as "I've read this thread" when it's +//! really "the oldest N of it" was the same silent trap `comments` had. +//! `--tail N` returns the last N (chronological) — and with neither flag +//! given, THIS is the default (N = 10): most-recent activity first, the +//! useful default for "what just happened here". `--since ` +//! filters to events 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 +//! before (see [`crate::verbs::MAX_LIMIT`]) since a since-filtered query +//! still has no total to page against precisely. //! -//! 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. -//! -//! `--limit` is capped at [`crate::verbs::MAX_LIMIT`]: the -//! over-fetch-by-one trick needs `limit + 1` to fit inside Forgejo's -//! 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`. +//! **The count of events outside whatever window is shown is always +//! reported** — `--tail`/default read the real total off this endpoint's +//! own `X-Total-Count` header (previously discarded; the "no total +//! exists" premise this module's docs used to state was false against +//! what `forgejo-api` actually returns), so unlike the old `--limit`-only +//! shape this is now an exact count, not a boolean "more" flag. use anyhow::Result; use clap::Args as ClapArgs; -use forgejo_api::structs::IssueGetCommentsAndTimelineQuery; +use forgejo_api::structs::{IssueGetCommentsAndTimelineQuery, TimelineComment}; use serde_json::{Value, json}; use crate::client::{Client, index}; -use crate::verbs::{MAX_LIMIT, clamp_limit, parse_rfc3339, print_json}; +use crate::verbs::{MAX_LIMIT, PAGE_SIZE, clamp_limit, parse_rfc3339, print_json}; #[derive(ClapArgs)] pub struct Args { /// Issue or PR number. pub(crate) number: u64, - /// 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, + /// Return the first `N` events (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`. + #[arg(long, conflicts_with = "tail")] + limit: Option, + /// Return the last `N` events (chronological) — the most recent + /// activity. This is the default (`N` = 10) when neither `--limit` + /// nor `--tail` is given. Mutually exclusive with `--limit`/`--since`. + #[arg(long, conflicts_with = "since")] + tail: Option, /// 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. @@ -51,32 +57,42 @@ pub struct Args { } 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); - 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)?, query) - .page_size(u32::try_from(fetch_limit).unwrap_or(u32::MAX)) - .send()?; - let limit = usize::try_from(limit).unwrap_or(usize::MAX); - let more = events.len() > limit; - let events: Vec<_> = events.into_iter().take(limit).collect(); + // Truncation info, same shape as `comments`: a `--tail`/default + // window has nothing after it; a `--limit` window has nothing + // before it; a `--since` window only knows a boolean "there's more" + // (no total for a since-filtered query). + let (events, 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 (events, more) = fetch_since(client, args.number, since, limit)?; + (events, 0, 0, more, clamped) + } else if let Some(limit) = args.limit { + let (events, total) = fetch_head(client, args.number, limit)?; + let more_after = total.saturating_sub(events.len()); + (events, 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 total = fetch_total(client, args.number)?; + let events = fetch_tail(client, args.number, n, total)?; + let more_before = total.saturating_sub(events.len()); + (events, more_before, 0, false, false) + }; // 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, "limit_clamped": clamped })); + return print_json(&json!({ + "events": v, + "more_before": more_before, + "more_after": more_after, + "since_more": since_more, + "since_limit_clamped": since_clamped, + })); } let Some(events) = v.as_array() else { return print_json(&v); @@ -84,18 +100,176 @@ pub fn run(client: &Client, args: Args) -> Result<()> { for ev in events { print_event(ev); } - if clamped { + if since_clamped { println!( - "(--limit {} is above the {MAX_LIMIT} cap this verb can reliably detect truncation at — clamped)", - args.limit + "(--limit {} is above the {MAX_LIMIT} cap --since can reliably detect truncation at — clamped)", + args.limit.unwrap_or(10) ); } - if more { - println!("(more activity not shown — raise --limit to see it; exact count not available)"); + if since_more { + println!( + "(more activity 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}"); } Ok(()) } +/// A one-line truncation note when the shown window doesn't cover the +/// whole timeline — never a silent gap. `None` when both are zero (the +/// whole timeline rode the wire). Mirrors `comments.rs`'s helper of the +/// same name/shape. +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 event(s) not shown — use --limit to see the start of the thread)" + )), + (false, true) => Some(format!( + "({more_after} more event(s) not shown — use --tail N to see the latest instead)" + )), + (true, true) => Some(format!( + "({more_before} earlier + {more_after} later event(s) not shown — use --tail N)" + )), + } +} + +/// Total timeline-event count for an issue/PR, read off this endpoint's +/// own `X-Total-Count` header via a cheap `page_size=1` request. Unlike +/// `comments.rs`'s `fetch_total` (which reads a field straight off the +/// issue object) there's no cheaper alternate endpoint carrying this +/// count — this request-then-discard-the-one-item IS the cheap path. +fn fetch_total(client: &Client, number: u64) -> Result { + let (owner, name) = client.owner_repo()?; + let (headers, _) = client + .api() + .issue_get_comments_and_timeline( + owner, + name, + index(number)?, + IssueGetCommentsAndTimelineQuery { + since: None, + before: None, + }, + ) + .page_size(1) + .send()?; + Ok(headers + .x_total_count + .and_then(|t| usize::try_from(t).ok()) + .unwrap_or(0)) +} + +/// Fetch the first page's worth of events (oldest-first) plus the +/// thread's real total, read straight off this same request's +/// `X-Total-Count` header — one request, no separate count query, and +/// (unlike the old over-fetch-by-one trick) an exact total rather than +/// a boolean "more". +fn fetch_head(client: &Client, number: u64, limit: u64) -> Result<(Vec, usize)> { + let (owner, name) = client.owner_repo()?; + let (limit, _clamped) = clamp_limit(limit); + let (headers, events) = client + .api() + .issue_get_comments_and_timeline( + owner, + name, + index(number)?, + IssueGetCommentsAndTimelineQuery { + since: None, + before: None, + }, + ) + .page_size(u32::try_from(limit).unwrap_or(u32::MAX)) + .send()?; + let total = headers + .x_total_count + .and_then(|t| usize::try_from(t).ok()) + .unwrap_or(events.len()); + Ok((events, total)) +} + +/// Fetch the last `n` timeline events (chronological). `total` (from +/// [`fetch_total`]) drives the pagination plan and is the caller's, not +/// fetched again here — same shape as `comments.rs`'s `fetch_tail`: +/// Forgejo paginates oldest-first with no `direction=desc` knob, so this +/// starts fetching from the page that contains item `total - n` rather +/// than paging the whole thread. +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 n = n.min(total); + 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; + let mut merged: Vec = Vec::with_capacity(n + page_size); + for page in start_page..=last_page { + let (_, arr) = client + .api() + .issue_get_comments_and_timeline( + owner, + name, + idx, + IssueGetCommentsAndTimelineQuery { + since: None, + before: None, + }, + ) + .page(u32::try_from(page).unwrap_or(u32::MAX)) + .page_size(u32::try_from(PAGE_SIZE).unwrap_or(u32::MAX)) + .send()?; + if arr.is_empty() { + // Page came back empty — either we miscounted (events added/ + // removed between the count request and now) or upstream's + // playing tricks. Stop rather than spin. + break; + } + merged.extend(arr); + } + let overshoot = merged.len().saturating_sub(n); + Ok(merged.into_iter().skip(overshoot).collect()) +} + +/// Fetch events 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, so this uses the same over-fetch-by-one trick +/// `comments.rs`'s `fetch_since` does: ask for `limit + 1`, and a full +/// extra row means there's more. Returns `(events, more)`, `more` a +/// boolean rather than an exact count. +fn fetch_since( + client: &Client, + number: u64, + since: time::OffsetDateTime, + limit: u64, +) -> Result<(Vec, bool)> { + let (owner, name) = client.owner_repo()?; + let query = IssueGetCommentsAndTimelineQuery { + since: Some(since), + before: None, + }; + let fetch_limit = limit.saturating_add(1); + let (_, events) = client + .api() + .issue_get_comments_and_timeline(owner, name, index(number)?, query) + .page_size(u32::try_from(fetch_limit).unwrap_or(u32::MAX)) + .send()?; + let limit = usize::try_from(limit).unwrap_or(usize::MAX); + let more = events.len() > limit; + let mut events = events; + events.truncate(limit); + Ok((events, more)) +} + /// Render one timeline event as a single `**actor @ ts**: summary` /// line. Comment rows inline their full body; structured event types /// (label, assignees, close, etc.) get a one-line human summary @@ -279,6 +453,68 @@ mod tests { //! duplicating the logic" — addressed. use super::*; + /// Pure helper mirroring the page-arithmetic in `fetch_tail`: given a + /// total event count + requested tail size, return the (`start_page`, + /// `last_page`) pair the network loop would walk. Same shape as + /// `comments.rs`'s `tail_plan` test helper — lets us pin the + /// pagination plan without touching the network. + fn tail_plan(total: usize, n: usize) -> (usize, usize) { + let n = n.min(total); + 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; + (start_page, last_page) + } + + #[test] + fn tail_plan_small_thread() { + assert_eq!(tail_plan(5, 2), (1, 1)); + } + + #[test] + fn tail_plan_crosses_page_boundary() { + // 51 total, tail 3 → start_idx=48 lives on page 1, item 51 lives + // on page 2; fetch both. + assert_eq!(tail_plan(51, 3), (1, 2)); + } + + #[test] + fn tail_plan_large_thread_bounded_pages() { + // 5000 total, tail 3 → ONE page fetch, not the whole thread. + assert_eq!(tail_plan(5000, 3), (100, 100)); + } + + #[test] + fn tail_plan_n_exceeds_total() { + assert_eq!(tail_plan(5, 100), (1, 1)); + } + + #[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_points_at_limit() { + let note = truncation_note(12, 0).unwrap(); + assert!(note.contains("12"), "{note}"); + assert!(note.contains("--limit"), "{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 comment_renders_body_inline() { let ev = serde_json::json!({