vinzenz/hyperhive
vinzenz/hyperhive
Watch
0
0
Fork
You've already forked hyperhive
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

hive-forge: comments/timeline default to newest, timeline gets a real total

Browse source 
Mara's decision on #4179: the default window for comments/timeline (no
flags at all) is now the newest 10, matching an explicit --tail 10 --
agent can still opt into the old oldest-first window via an explicit
--limit. Same restructure in both verbs: limit u64 -> Option<u64>, only
the explicit-limit branch takes the head path now, the tail-or-default
branch covers both --tail N and the no-flags case.

timeline.rs also gets the real fix: TimelineListHeaders.x_total_count is
a genuine field forgejo-api already returns on every timeline call --
verified against the vendored crate source. The old 'no total exists'
premise in the module doc was false, same shape as #3200. Replaced the
over-fetch-by-one boolean 'more' with an exact count via a cheap
page_size=1 fetch_total, mirroring comments.rs's fetch_tail pagination
math for --tail. JSON output reshaped to {events, more_before,
more_after, since_more, since_limit_clamped}, matching comments.rs --
a breaking interface change, intentional.

Also fixed a real pre-existing bug in comments.rs's truncation_note:
the more_before message suggested retrying with --tail, which is
nonsensical since more_before only ever fires from a tail-shaped
window. Now correctly suggests --limit. Added a regression test.

fixes #4179
This commit is contained in:
damocles 2026-09-11 12:55:48 +02:00 • committed by mara
commit 1be4b81f65
2 changed files with 333 additions and 85 deletions
Download patch file Download diff file Expand all files Collapse all files

70
hive-forge/src/verbs/comments.rs
View file

@ -1,15 +1,15 @@
//! `comments <number> [--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 <RFC3339>` 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<u64>,
/// 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<usize>,
/// 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<String> {
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]

348
hive-forge/src/verbs/timeline.rs
View file

@ -1,48 +1,54 @@
//! `timeline <number> [--limit N]` — list timeline events on an
//! issue or PR. Fills the gap where agents kept falling back to curl
//! `timeline <number> [--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 <n>` / `comments <n>` — 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 <RFC3339>`
//! 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 <RFC3339>` 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<u64>,
/// 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<usize>,
/// 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<String> {
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<usize> {
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<TimelineComment>, 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<Vec<TimelineComment>> {
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<TimelineComment> = 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<TimelineComment>, 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!({