diff --git a/hive-forge/src/verbs/comments.rs b/hive-forge/src/verbs/comments.rs index 585a1582..17bde9e6 100644 --- a/hive-forge/src/verbs/comments.rs +++ b/hive-forge/src/verbs/comments.rs @@ -1,92 +1,65 @@ //! `comments [--limit N | --tail N]` — list comments on an -//! issue or PR. Replaces the curl fallback. +//! issue or PR. Replaces the curl fallback; `--tail` +//! handles the paging-for-long-threads +//! awkwardness). //! -//! - `--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. -//! - `--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`). +//! - `--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. //! -//! 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]`. +//! 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. //! -//! `--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. +//! Use the global `--json` flag for JSON output. 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::{MAX_LIMIT, PAGE_SIZE, clamp_limit, parse_rfc3339, print_json, rfc3339}; +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; #[derive(ClapArgs)] 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")] + /// Number of comments from the start of the thread (max 50). + /// Mutually exclusive with `--tail`. + #[arg(long, default_value_t = 50, conflicts_with = "tail")] limit: u64, /// Return the last `N` comments (chronological). Mutually exclusive - /// 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`. + /// with `--limit`. #[arg(long)] - since: Option, + tail: Option, } 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 - // 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, 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) - }; + let thread = match args.tail { + Some(n) => fetch_tail(client, args.number, n)?, + None => fetch_head(client, args.number, args.limit)?, + }; // 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)); @@ -109,13 +82,7 @@ pub fn run(client: &Client, args: Args) -> Result<()> { }) }) .collect(); - print_json(&json!({ - "comments": trimmed, - "more_before": more_before, - "more_after": more_after, - "since_more": since_more, - "since_limit_clamped": since_clamped, - })) + print_json(&Value::Array(trimmed)) } else { for c in &comments { let user = c @@ -133,58 +100,10 @@ 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}"); - } 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). @@ -278,26 +197,32 @@ 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: 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 { +/// 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 { 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); - let page_size = usize::try_from(PAGE_SIZE).unwrap_or(usize::MAX); + let page_size = PAGE_SIZE; // 0-based index of the first comment we want; integer-divide to // get the 1-based page that contains it. let start_idx = total - n; @@ -326,36 +251,6 @@ fn fetch_tail(client: &Client, number: u64, n: usize, total: usize) -> 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::*; @@ -367,7 +262,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 = usize::try_from(PAGE_SIZE).unwrap_or(usize::MAX); + let page_size = PAGE_SIZE; let start_idx = total - n; let start_page = (start_idx / page_size) + 1; let last_page = (total - 1) / page_size + 1; @@ -435,31 +330,6 @@ 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/mod.rs b/hive-forge/src/verbs/mod.rs index e86ed8ab..f2cd9bc8 100644 --- a/hive-forge/src/verbs/mod.rs +++ b/hive-forge/src/verbs/mod.rs @@ -69,56 +69,6 @@ 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 cc8dc09e..2f976115 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,73 +10,46 @@ //! a human-readable form by default; pass the global `--json` flag //! for the raw API shape. //! -//! 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`. +//! `--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). use anyhow::Result; use clap::Args as ClapArgs; use forgejo_api::structs::IssueGetCommentsAndTimelineQuery; -use serde_json::{Value, json}; +use serde_json::Value; use crate::client::{Client, index}; -use crate::verbs::{MAX_LIMIT, clamp_limit, parse_rfc3339, print_json}; +use crate::verbs::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)] + /// Page size (Forgejo caps at 50). Returns the first `N` events. + #[arg(long, default_value_t = 50)] limit: u64, - /// 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<()> { 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)) + .issue_get_comments_and_timeline( + owner, + name, + index(args.number)?, + IssueGetCommentsAndTimelineQuery::default(), + ) + .page_size(u32::try_from(args.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(); // 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(&json!({ "events": v, "more": more, "limit_clamped": clamped })); + return print_json(&v); } let Some(events) = v.as_array() else { return print_json(&v); @@ -84,15 +57,6 @@ pub fn run(client: &Client, args: Args) -> Result<()> { for ev in events { print_event(ev); } - if clamped { - println!( - "(--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(()) }