diff --git a/hive-forge/src/verbs/comments.rs b/hive-forge/src/verbs/comments.rs index b2095675..3c47181b 100644 --- a/hive-forge/src/verbs/comments.rs +++ b/hive-forge/src/verbs/comments.rs @@ -7,11 +7,13 @@ //! 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. +//! activity. Reads this endpoint's own real total first (see +//! `fetch_total`) 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. +//! always reported** — never a silent truncation. That total is this +//! endpoint's own header, never `issue_get_issue`'s `comments` field +//! (see `fetch_total`'s doc comment for why). //! - `--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. Mutually @@ -20,8 +22,7 @@ //! //! Review *bodies* on PRs are always merged in too (`pulls//reviews` //! isn't the issues/comments thread), tagged `[review: STATE]`, with a -//! `(N line comment(s) — see 'pr reviews')` pointer when a review has -//! inline comments — this verb never inlines those, `pr reviews` does. +//! `(N line comment(s) — see 'pr reviews')` pointer for inline comments. //! //! `--json` output is an object (`{"comments": [...], "more_before": N, //! "more_after": N, "since_more": bool}`), not a bare array, so a @@ -86,8 +87,7 @@ pub fn run(client: &Client, args: Args) -> Result<()> { let (thread, more) = fetch_since(client, args.number, since, limit)?; (thread, 0, 0, more, clamped) } else if let Some(limit) = args.limit { - let total = fetch_total(client, args.number)?; - let thread = fetch_head(client, args.number, limit)?; + let (thread, total) = fetch_head(client, args.number, limit)?; let more_after = total.saturating_sub(thread.len()); (thread, 0, more_after, false, false) } else { @@ -276,19 +276,48 @@ fn attachment_json_from_value(c: &Value) -> Vec { .collect() } -/// Total comment count on an issue/PR, straight off the issue object. -/// Used both to plan `--tail`'s pagination and to report how many +/// Interpret a comments-endpoint response's `X-Total-Count` header as +/// the real prose-comment total, falling back to `fallback` (the +/// window just fetched, or `0` for a bare probe) when the header is +/// absent or won't fit a `usize` (defensive — Forgejo always sends a +/// small non-negative value in practice). Never invents a number: a +/// missing header only ever *under*-reports via the fallback, it can't +/// manufacture a `more_before`/`more_after` count that then turns out +/// to be a non-prose event the comments endpoint would never surface. +fn total_from_header(x_total_count: Option, fallback: usize) -> usize { + x_total_count + .and_then(|t| usize::try_from(t).ok()) + .unwrap_or(fallback) +} + +/// Real *prose*-comment count on an issue/PR, read off this same +/// endpoint's own `X-Total-Count` header via a cheap `page_size=1` +/// probe — used to plan `--tail`'s pagination and to report how many /// comments sit outside whatever window ends up shown. +/// +/// This used to read `issue_get_issue`'s `comments` field instead — +/// Forgejo's running counter on the Issue object, which also increments for +/// several non-prose event kinds (`review_request`, `issue_ref`, +/// `comment_ref`, `pull_push`, …) that `issue_get_comments` never +/// returns. Subtracting that inflated count against this endpoint's own +/// results reported drift that was never real: a comment that would +/// never be shown at any window size. Mirrors `timeline.rs`'s own +/// `fetch_total`, which moved to the same trick for its own endpoint +/// first — reading the count straight off the endpoint that also +/// produces the window means the two can never drift apart again. fn fetch_total(client: &Client, number: u64) -> Result { let (owner, name) = client.owner_repo()?; - let issue = client + let (headers, _) = client .api() - .issue_get_issue(owner, name, index(number)?) + .issue_get_comments( + owner, + name, + index(number)?, + IssueGetCommentsQuery::default(), + ) + .page_size(1) .send()?; - Ok(issue - .comments - .and_then(|c| usize::try_from(c).ok()) - .unwrap_or(0)) + Ok(total_from_header(headers.x_total_count, 0)) } /// Serialize a typed comment page back to the JSON `Value` shape the @@ -374,10 +403,13 @@ fn merge_chronological(mut items: Vec, reviews: Vec) -> Vec items } -/// Fetch the first page's worth of comments (existing behaviour). -fn fetch_head(client: &Client, number: u64, limit: u64) -> Result> { +/// Fetch the first page's worth of comments (existing behaviour) plus +/// the thread's real total, read straight off this same request's +/// `X-Total-Count` header — one request, no separate `fetch_total` +/// call needed for this branch. +fn fetch_head(client: &Client, number: u64, limit: u64) -> Result<(Vec, usize)> { let (owner, name) = client.owner_repo()?; - let (_, comments) = client + let (headers, comments) = client .api() .issue_get_comments( owner, @@ -387,7 +419,8 @@ fn fetch_head(client: &Client, number: u64, limit: u64) -> Result> { ) .page_size(u32::try_from(limit).unwrap_or(u32::MAX)) .send()?; - to_values(comments) + let total = total_from_header(headers.x_total_count, comments.len()); + Ok((to_values(comments)?, total)) } /// Fetch the last `n` comments on an issue/PR in chronological order. @@ -530,6 +563,66 @@ mod tests { assert_eq!(tail_plan(5, 100), (1, 1)); } + #[test] + fn tail_plan_with_inflated_total_overshoots_the_real_last_page() { + // The offset half of the mismatched-total mechanism: 48 real prose comments + // (all fit on page 1, page_size 50) plus 5 non-prose events the + // old `issue.comments` counter double-counted gives an inflated + // total of 53. The correct plan never leaves page 1 — everything + // real lives there. + assert_eq!(tail_plan(48, 3), (1, 1)); + // Fed the inflated total instead, the plan reaches for page 2, + // which doesn't exist on the real thread — `fetch_tail`'s + // `arr.is_empty()` guard then breaks immediately and returns + // zero comments instead of the real last 3. Not just a wrong + // trailer number: the wrong rows (none) get shown. + assert_eq!(tail_plan(53, 3), (2, 2)); + } + + #[test] + fn total_from_header_prefers_the_header_over_the_fallback() { + assert_eq!(total_from_header(Some(53), 8), 53); + } + + #[test] + fn total_from_header_falls_back_when_header_absent() { + // Never invents a number it doesn't have — under-reports to the + // fallback rather than guessing. + assert_eq!(total_from_header(None, 8), 8); + } + + #[test] + fn total_from_header_falls_back_on_a_malformed_value() { + // A negative count can't be a real `usize`; fall back rather + // than silently wrapping. + assert_eq!(total_from_header(Some(-1), 8), 8); + } + + #[test] + fn drift_scenario_reports_nothing_withheld_when_header_matches_window() { + // The core mismatched-total scenario: the real total, read off the comments + // endpoint's own header, matches the fetched window exactly, even + // though a differently-scoped counter (the old `issue.comments` + // field, no longer read) would have come back higher because of + // unrelated non-prose events (`review_request`, `issue_ref`, + // `comment_ref`, …) that endpoint never returns. Trailer must + // report nothing withheld. + let window_len = 8; + let total = total_from_header(Some(8), window_len); + assert_eq!(truncation_note(total.saturating_sub(window_len), 0), None); + } + + #[test] + fn drift_scenario_still_reports_a_genuine_truncation() { + // A real truncation must still surface — silencing the trailer + // outright would swap a false positive for a false negative, + // which is strictly worse. + let window_len = 10; + let total = total_from_header(Some(12), window_len); + let note = truncation_note(total.saturating_sub(window_len), 0).unwrap(); + assert!(note.contains('2'), "{note}"); + } + #[test] fn merge_interleaves_reviews_by_timestamp() { // A review submitted between two comments must land between