diff --git a/docs/integrations/forge.md b/docs/integrations/forge.md index a048a925..74f2d165 100644 --- a/docs/integrations/forge.md +++ b/docs/integrations/forge.md @@ -303,19 +303,20 @@ with the meta-suffix `reviewer:` line (requested reviewers, below). A review's line comments belong to neither the review body nor the `latest_comment_url` (a review with only inline comments has no such url and otherwise lands as a bare `activity on PR`). So every PR wake fetches -the PR's reviews (`/pulls/N/reviews`, up to four pages of 50) and appends -the inline comments of those that are new to the agent as an -`inline review comments:` block, one `author [path:line]: text` entry -each (`[path]` when Forgejo dropped the position because the line left the -diff). A review counts as new when it was submitted strictly after the -thread's previously delivered `updated_at` (all of them without a -baseline, and any with no submit time, so nothing is hidden), is not the -agent's own, is not a `PENDING` draft, and has `comments_count > 0`. Only -the newest 5 such reviews are fetched, at most 10 comments are shown -(`(+N more ...)` for the rest), and each excerpt is truncated to 300 bytes -and heading-escaped like a body. Best effort: a failed fetch yields less -text, never a dropped wake. Cost: one reviews fetch per PR wake, plus one -per new commented review. +the PR's reviews (`/pulls/N/reviews`, up to four pages of 50) and, for the +reviews new to the agent, appends an +`inline review comments (read them with `hive-forge pr reviews N`):` block +saying where they are: one `author: path:line, path, ...` line per review +author (bare `path` when Forgejo dropped the position because the line left +the diff; control characters in a path become spaces). The comment text stays +out of the wake - the agent reads the full review with the CLI. A review +counts as new when it was submitted strictly after the thread's previously +delivered `updated_at` (all of them without a baseline, and any with no +submit time, so nothing is hidden), is not the agent's own, is not a +`PENDING` draft, and has `comments_count > 0`. Only the newest 5 such reviews +are fetched and at most 10 locations are listed (`(+N more)` for the rest). +Best effort: a failed fetch yields less text, never a dropped wake. Cost: one +reviews fetch per PR wake, plus one per new commented review. ### Merge/close vs a later comment diff --git a/hive-forge-notify/src/notify.rs b/hive-forge-notify/src/notify.rs index 2a7dc021..d6f829b0 100644 --- a/hive-forge-notify/src/notify.rs +++ b/hive-forge-notify/src/notify.rs @@ -900,10 +900,8 @@ async fn fresh_post_close_comment_tail( Some(format!("\n\ncomment by {author}: {excerpt}{mentions}")) } -/// Inline review comments rendered in one wake; the rest become a count. +/// Inline review comment locations listed in one wake; the rest become a count. const INLINE_COMMENTS_MAX: usize = 10; -/// Per-comment excerpt cap. Under [`BODY_TRUNCATE`]: one review carries many. -const INLINE_COMMENT_TRUNCATE: usize = 300; /// Reviews whose inline comments are fetched for one wake (the newest). const INLINE_REVIEWS_MAX: usize = 5; /// Page size and page cap for the reviews list, so a long-lived PR's @@ -911,12 +909,12 @@ const INLINE_REVIEWS_MAX: usize = 5; const REVIEWS_PAGE_SIZE: usize = 50; const REVIEWS_PAGES_MAX: usize = 4; -/// The PR's reviews endpoint from the notification subject URL. Forgejo -/// hands out the *issues* URL for a PR (`.../issues/N`); its reviews live -/// under `.../pulls/N/reviews`. `None` for any other shape. -fn reviews_api_url(subject_api_url: &str) -> Option { +/// The PR's reviews endpoint and number from the notification subject URL. +/// Forgejo hands out the *issues* URL for a PR (`.../issues/N`); its reviews +/// live under `.../pulls/N/reviews`. `None` for any other shape. +fn reviews_api_url(subject_api_url: &str) -> Option<(String, &str)> { let (head, number) = subject_api_url.rsplit_once("/issues/")?; - Some(format!("{head}/pulls/{number}/reviews")) + Some((format!("{head}/pulls/{number}/reviews"), number)) } /// `(review id, author)` of the reviews in `reviews` (the raw reviews @@ -954,40 +952,46 @@ fn unseen_inline_reviews( unseen } -/// The `inline review comments:` block for the wake: one -/// `author [path:line]: text` entry per comment (`[path]` when Forgejo -/// dropped the position because the line left the diff), each excerpt -/// truncated + heading-escaped like a body, at most -/// [`INLINE_COMMENTS_MAX`] entries and a count for the rest. Empty when -/// there is nothing to show. -fn render_inline_comments(groups: &[(String, Vec)]) -> String { +/// One comment's location for the wake: `path:line`, or just `path` when +/// Forgejo dropped the position because the line left the diff. Control +/// characters in the path (git allows a newline in one) become spaces so a +/// PR author cannot shape the lines the agent reads. +fn comment_location(c: &serde_json::Value) -> String { + let path: String = c["path"] + .as_str() + .unwrap_or("?") + .chars() + .map(|ch| if ch.is_control() { ' ' } else { ch }) + .collect(); + match c["position"].as_u64().filter(|&line| line > 0) { + Some(line) => format!("{path}:{line}"), + None => path, + } +} + +/// The `inline review comments` block for the wake: only where the unseen +/// comments are, one line per review author (`author: path:line, ...`), +/// capped at [`INLINE_COMMENTS_MAX`] locations in total with a count for the +/// rest. The text stays out of the wake: the pointer names the CLI call that +/// prints the full review. Empty when there is nothing to show. +fn render_inline_comments(pr_number: &str, groups: &[(String, Vec)]) -> String { let total: usize = groups.iter().map(|(_, c)| c.len()).sum(); if total == 0 { return String::new(); } - let mut out = String::from("\n\ninline review comments:"); - let entries = groups - .iter() - .flat_map(|(author, comments)| comments.iter().map(move |c| (author, c))); - for (author, c) in entries.take(INLINE_COMMENTS_MAX) { - let path = c["path"].as_str().unwrap_or("?"); - let body = escape_md_headings(&truncate( - c["body"].as_str().unwrap_or("").trim(), - INLINE_COMMENT_TRUNCATE, - )) - .replace('\n', "\n "); - match c["position"].as_u64().filter(|&line| line > 0) { - Some(line) => write!(out, "\n {author} [{path}:{line}]: {body}").ok(), - None => write!(out, "\n {author} [{path}]: {body}").ok(), - }; + let mut out = + format!("\n\ninline review comments (read them with `hive-forge pr reviews {pr_number}`):"); + let mut budget = INLINE_COMMENTS_MAX; + for (author, comments) in groups { + let shown: Vec = comments.iter().take(budget).map(comment_location).collect(); + if shown.is_empty() { + continue; + } + budget -= shown.len(); + write!(out, "\n {author}: {}", shown.join(", ")).ok(); } if total > INLINE_COMMENTS_MAX { - write!( - out, - "\n (+{} more - `hive-forge pr reviews` lists them all)", - total - INLINE_COMMENTS_MAX - ) - .ok(); + write!(out, "\n (+{} more)", total - INLINE_COMMENTS_MAX).ok(); } out } @@ -1002,7 +1006,7 @@ async fn fetch_inline_review_tail( prev_updated_at: Option<&str>, own_login: &str, ) -> String { - let Some(reviews_url) = reviews_api_url(subject_api_url) else { + let Some((reviews_url, pr_number)) = reviews_api_url(subject_api_url) else { return String::new(); }; let mut reviews: Vec = Vec::new(); @@ -1027,7 +1031,7 @@ async fn fetch_inline_review_tail( groups.push((author, comments.clone())); } } - render_inline_comments(&groups) + render_inline_comments(pr_number, &groups) } /// Parse an RFC 3339 timestamp as Forgejo emits them @@ -1997,8 +2001,8 @@ mod tests { fn reviews_api_url_swaps_issues_for_pulls() { let subject = "/api/v1/repos/o/r/issues/42"; // lint:allow test input, not a tracker reference assert_eq!( - reviews_api_url(subject).as_deref(), - Some("/api/v1/repos/o/r/pulls/42/reviews"), + reviews_api_url(subject), + Some(("/api/v1/repos/o/r/pulls/42/reviews".to_owned(), "42")), ); assert_eq!(reviews_api_url("/api/v1/repos/o/r"), None); } @@ -2052,39 +2056,57 @@ mod tests { } #[test] - fn render_inline_comments_formats_path_line_and_fallback() { - let groups = vec![( - "argus".to_owned(), - vec![ - serde_json::json!({"path": "src/a.rs", "position": 42, "body": "off by one"}), - serde_json::json!({"path": "src/b.rs", "position": 0, "body": "line left the diff"}), - serde_json::json!({"path": "src/c.rs", "body": "# heading\nsecond line"}), - ], - )]; + fn render_inline_comments_lists_locations_not_text() { + let groups = vec![ + ( + "argus".to_owned(), + vec![ + serde_json::json!({"path": "src/a.rs", "position": 42, "body": "off by one"}), + serde_json::json!({"path": "src/b.rs", "position": 0, "body": "left the diff"}), + ], + ), + ( + "mara".to_owned(), + vec![serde_json::json!({"path": "src/c.rs", "position": 7, "body": "why"})], + ), + ]; assert_eq!( - render_inline_comments(&groups), - "\n\ninline review comments:\ - \n argus [src/a.rs:42]: off by one\ - \n argus [src/b.rs]: line left the diff\ - \n argus [src/c.rs]: \\# heading\n second line" + render_inline_comments("12", &groups), + "\n\ninline review comments (read them with `hive-forge pr reviews 12`):\ + \n argus: src/a.rs:42, src/b.rs\ + \n mara: src/c.rs:7" ); } #[test] fn render_inline_comments_empty_is_empty() { - assert_eq!(render_inline_comments(&[]), ""); - assert_eq!(render_inline_comments(&[("argus".to_owned(), vec![])]), ""); + assert_eq!(render_inline_comments("12", &[]), ""); + assert_eq!( + render_inline_comments("12", &[("argus".to_owned(), vec![])]), + "" + ); } #[test] - fn render_inline_comments_caps_entries_and_excerpts() { - let comments: Vec<_> = (0..13) - .map(|i| serde_json::json!({"path": "f.rs", "position": i + 1, "body": "x".repeat(1000)})) - .collect(); - let out = render_inline_comments(&[("argus".to_owned(), comments)]); - assert_eq!(out.matches("argus [f.rs:").count(), INLINE_COMMENTS_MAX); - assert!(out.ends_with("(+3 more - `hive-forge pr reviews` lists them all)")); - // Each excerpt is cut, not the full 1000 chars. - assert!(out.len() < INLINE_COMMENTS_MAX * (INLINE_COMMENT_TRUNCATE + 60) + 200); + fn render_inline_comments_caps_locations_across_authors() { + let many = |n: u64| -> Vec { + (1..=n) + .map(|i| serde_json::json!({"path": "f.rs", "position": i})) + .collect() + }; + let out = render_inline_comments( + "12", + &[("argus".to_owned(), many(8)), ("mara".to_owned(), many(5))], + ); + // 8 from argus, the 2 left of the budget from mara, 3 counted. + assert_eq!(out.matches("f.rs:").count(), INLINE_COMMENTS_MAX); + assert!(out.contains("\n mara: f.rs:1, f.rs:2\n")); + assert!(out.ends_with("\n (+3 more)")); + } + + #[test] + fn comment_location_neutralises_control_characters_in_the_path() { + let c = serde_json::json!({"path": "a\nb\tc.rs", "position": 3}); + assert_eq!(comment_location(&c), "a b c.rs:3"); } }