diff --git a/docs/4-implementation/agent-tools/forge-cli.md b/docs/4-implementation/agent-tools/forge-cli.md index 4102fddc5..bfa05cba0 100644 --- a/docs/4-implementation/agent-tools/forge-cli.md +++ b/docs/4-implementation/agent-tools/forge-cli.md @@ -609,6 +609,7 @@ List a PR's reviews, or submit one * `--request-changes` — Request changes on the PR (submit a `REQUEST_CHANGES` review) * `--comment` — Leave a comment review (submit a COMMENT review) * `-m`, `--body ` — Optional body / message for the review (used with --approve, --request-changes, or --comment) +* `--at ` — An inline comment on a line of the new file: `--at path:LINE "text"`. Repeat it to put several comments in the same review. LINE is the new-file line number, which `pr diff --numbered` prints. Needs --approve, --request-changes or --comment * `--only ` — When listing: narrow the inline comments shown (and leave out the reviews with none left). `unresolved` keeps every comment nobody has resolved yet: what a re-review has to check Possible values: @@ -666,6 +667,7 @@ Print the PR's unified diff ###### **Options:** * `--full` — Print the unfiltered diff. By default, hive-forge collapses autogenerated-file hunks (`flake.lock`, `Cargo.lock`, …) to a placeholder so human-authored changes stay readable +* `--numbered` — Prefix every added or unchanged line with its new-file line number, the number `pr reviews --at path:LINE` takes. Removed lines get no number: an inline comment sits on the new file diff --git a/docs/4-implementation/agent-tools/forge.md b/docs/4-implementation/agent-tools/forge.md index 19aed65ea..d03829f09 100644 --- a/docs/4-implementation/agent-tools/forge.md +++ b/docs/4-implementation/agent-tools/forge.md @@ -83,11 +83,13 @@ hive-forge pr timeline 42 # review rows list their line comment hive-forge pr reviews 42 --approve # submit APPROVED review hive-forge pr reviews 42 --request-changes -m "msg" # submit REQUEST_CHANGES review hive-forge pr reviews 42 --comment -m "msg" # submit COMMENT review +hive-forge pr reviews 42 --request-changes -m "msg" --at src/a.rs:12 "off by one" --at src/b.rs:3 "why?" # one review carrying line comments; LINE is the new-file line `pr diff --numbered` prints hive-forge pr assign-reviewer 42 argus # set argus as a requested reviewer on the PR hive-forge pr assign-reviewer 42 argus --remove # withdraw the request; fails if argus is still listed afterwards (a reviewer who already reviewed stays listed) hive-forge pr assign-committer 42 damocles # add damocles to the PR's assignee list hive-forge pr diff 42 # unified diff (lockfile hunks collapsed by default) hive-forge pr diff 42 --full # include unfiltered lockfile hunks +hive-forge pr diff 42 --numbered # prefix added/unchanged lines with their new-file line number, for `pr reviews --at path:LINE` hive-forge list # open issues/PRs hive-forge list --kind pr --state all --page 2 # page 2 of all PRs (walk --page 1,2,… with --limit as page size for a repo-wide sweep) hive-forge list --search "trust bundle" --state all # full-text over title AND body — the duplicate check diff --git a/hive-forge/src/verbs/diff.rs b/hive-forge/src/verbs/diff.rs index 5a2632c4a..6a5e90c0d 100644 --- a/hive-forge/src/verbs/diff.rs +++ b/hive-forge/src/verbs/diff.rs @@ -13,6 +13,8 @@ //! lockfile without any information the reader can act on. //! Pass `--full` to dump the unfiltered diff. +use std::fmt::Write as _; + use anyhow::Result; use clap::Args as ClapArgs; use forgejo_api::structs::RepoDownloadPullDiffOrPatchQuery; @@ -29,6 +31,11 @@ pub struct Args { /// placeholder so human-authored changes stay readable. #[arg(long)] full: bool, + /// Prefix every added or unchanged line with its new-file line number, + /// the number `pr reviews --at path:LINE` takes. Removed lines get no + /// number: an inline comment sits on the new file. + #[arg(long)] + numbered: bool, } pub fn run(client: &Client, args: Args) -> Result<()> { @@ -49,10 +56,57 @@ pub fn run(client: &Client, args: Args) -> Result<()> { } else { collapse_autogenerated(&diff) }; - print!("{out}"); + if args.numbered { + print!("{}", number_new_lines(&out)); + } else { + print!("{out}"); + } Ok(()) } +/// Width of the line-number column `number_new_lines` adds. +const NUMBER_WIDTH: usize = 6; + +/// Prefix each line of every hunk with its new-file line number (added and +/// unchanged lines) or blanks (removed lines and `\ No newline` markers), so a +/// reviewer can read the `LINE` of `pr reviews --at path:LINE` off the diff. +/// Everything outside a hunk (git headers, collapsed placeholders) is passed +/// through unchanged. +fn number_new_lines(diff: &str) -> String { + let blank = " ".repeat(NUMBER_WIDTH); + let mut out = String::with_capacity(diff.len() + diff.len() / 8); + let mut next: Option = None; + for line in diff.lines() { + if let Some(rest) = line.strip_prefix("@@ ") { + next = hunk_new_start(rest); + out.push_str(line); + } else if line.starts_with("diff --git ") { + next = None; + out.push_str(line); + } else if let Some(n) = next { + if line.starts_with('-') || line.starts_with('\\') { + out.push_str(&blank); + } else { + let _ = write!(out, "{n:>NUMBER_WIDTH$}"); + next = Some(n + 1); + } + out.push(' '); + out.push_str(line); + } else { + out.push_str(line); + } + out.push('\n'); + } + out +} + +/// The new-file start line of a hunk header, from `-a,b +c,d @@ ...` (the +/// text after the leading `@@ `): `c`. +fn hunk_new_start(header: &str) -> Option { + let new = header.split_whitespace().find(|w| w.starts_with('+'))?; + new.trim_start_matches('+').split(',').next()?.parse().ok() +} + /// Walk a unified diff line-by-line. When a `diff --git` line /// names a known autogenerated file (`is_autogenerated`), suppress /// every line of that per-file section (the git headers AND the @@ -224,6 +278,58 @@ fn is_autogenerated(path: &str) -> bool { mod tests { use super::*; + const TWO_FILES: &str = "\ +diff --git a/a.rs b/a.rs +index 1..2 100644 +--- a/a.rs ++++ b/a.rs +@@ -10,4 +20,5 @@ fn f() { + keep +-gone ++new one ++new two + tail +\\ No newline at end of file +diff --git a/b.rs b/b.rs +--- a/b.rs ++++ b/b.rs +@@ -1 +1 @@ +-x ++y +"; + + #[test] + fn new_file_lines_are_numbered_from_the_hunk_header() { + let got = number_new_lines(TWO_FILES); + let expected = "\ +diff --git a/a.rs b/a.rs +index 1..2 100644 +--- a/a.rs ++++ b/a.rs +@@ -10,4 +20,5 @@ fn f() { + 20 keep + -gone + 21 +new one + 22 +new two + 23 tail + \\ No newline at end of file +diff --git a/b.rs b/b.rs +--- a/b.rs ++++ b/b.rs +@@ -1 +1 @@ + -x + 1 +y +"; + assert_eq!(got, expected); + } + + #[test] + fn a_hunk_header_without_a_comma_still_gives_its_start() { + assert_eq!(hunk_new_start("-1 +7 @@"), Some(7)); + assert_eq!(hunk_new_start("-3,2 +9,4 @@ ctx"), Some(9)); + assert_eq!(hunk_new_start("garbage"), None); + } + #[test] fn detects_known_lockfiles() { assert!(is_autogenerated("flake.lock")); diff --git a/hive-forge/src/verbs/pr_reviews.rs b/hive-forge/src/verbs/pr_reviews.rs index 07d1219c1..833ff3ee2 100644 --- a/hive-forge/src/verbs/pr_reviews.rs +++ b/hive-forge/src/verbs/pr_reviews.rs @@ -1,11 +1,11 @@ //! `pr reviews ` — list reviews, or submit one via `--approve` / -//! `--request-changes` / `--comment`. +//! `--request-changes` / `--comment`, optionally with inline `--at` comments. use std::fmt::Write as _; -use anyhow::{Result, bail}; +use anyhow::{Context as _, Result, bail}; use clap::Args as ClapArgs; -use forgejo_api::structs::CreatePullReviewOptions; +use forgejo_api::structs::{CreatePullReviewComment, CreatePullReviewOptions}; use serde_json::{Value, json}; use crate::client::{Client, index}; @@ -33,6 +33,19 @@ pub struct Args { #[arg(long, short = 'm')] body: Option, + /// An inline comment on a line of the new file: `--at path:LINE "text"`. + /// Repeat it to put several comments in the same review. LINE is the + /// new-file line number, which `pr diff --numbered` prints. Needs + /// --approve, --request-changes or --comment. + #[arg( + long, + num_args = 2, + value_names = ["PATH:LINE", "TEXT"], + action = clap::ArgAction::Append, + conflicts_with = "only", + )] + at: Vec, + /// When listing: narrow the inline comments shown (and leave out the /// reviews with none left). `unresolved` keeps every comment nobody has /// resolved yet: what a re-review has to check. @@ -61,22 +74,65 @@ pub fn run(client: &Client, args: Args) -> Result<()> { }; if let Some(ev) = event { - submit_review(client, args.number, ev, args.body) + let comments = parse_inline_comments(&args.at)?; + submit_review(client, args.number, ev, args.body, comments) } else { if args.body.is_some() { bail!("--body requires one of --approve / --request-changes / --comment"); } + if !args.at.is_empty() { + bail!("--at requires one of --approve / --request-changes / --comment"); + } list_reviews(client, args.number, args.only) } } -/// Submit a review event (`APPROVED` / `REQUEST_CHANGES` / `COMMENT`) and print -/// a compact summary of the created review. -fn submit_review(client: &Client, number: u64, event: &str, body: Option) -> Result<()> { +/// Turn the `--at` values (flat `PATH:LINE`, `TEXT` pairs) into the inline +/// comments of a review. Lines are new-file lines. +fn parse_inline_comments(at: &[String]) -> Result> { + at.chunks(2) + .map(|pair| { + let [location, text] = pair else { + bail!("--at takes a PATH:LINE and a TEXT"); + }; + let (path, line) = location + .rsplit_once(':') + .with_context(|| format!("--at {location:?}: expected PATH:LINE"))?; + let line: i64 = + line.parse().ok().filter(|l| *l > 0).with_context(|| { + format!("--at {location:?}: LINE must be a positive number") + })?; + if path.is_empty() { + bail!("--at {location:?}: PATH is empty"); + } + if text.trim().is_empty() { + bail!("--at {location:?}: the comment text is empty"); + } + Ok(CreatePullReviewComment { + body: Some(text.clone()), + extra_lines_count: None, + new_position: Some(line), + old_position: Some(0), + path: Some(path.to_owned()), + }) + }) + .collect() +} + +/// Submit a review event (`APPROVED` / `REQUEST_CHANGES` / `COMMENT`), with +/// its inline `comments`, and print a compact summary of the created review. +fn submit_review( + client: &Client, + number: u64, + event: &str, + body: Option, + comments: Vec, +) -> Result<()> { let (owner, name) = client.owner_repo()?; + let n_comments = comments.len(); let payload = CreatePullReviewOptions { body: Some(body.unwrap_or_default()), - comments: None, + comments: (!comments.is_empty()).then_some(comments), commit_id: None, event: Some(event.to_owned()), }; @@ -88,6 +144,7 @@ fn submit_review(client: &Client, number: u64, event: &str, body: Option "id": review.id, "state": review.state, "user": review.user.as_ref().and_then(|u| u.login.as_deref()), + "inline_comments": n_comments, })) } @@ -334,6 +391,54 @@ fn list_reviews_text(client: &Client, number: u64, reviews: &[Value], only: Opti mod tests { use super::*; + fn at(pairs: &[(&str, &str)]) -> Vec { + pairs + .iter() + .flat_map(|(loc, text)| [(*loc).to_owned(), (*text).to_owned()]) + .collect() + } + + #[test] + fn at_pairs_become_new_file_line_comments() { + let got = parse_inline_comments(&at(&[ + ("src/a.rs:12", "off by one"), + ("docs/x:y.md:3", "colon in the path"), + ])) + .unwrap(); + assert_eq!(got.len(), 2); + assert_eq!(got[0].path.as_deref(), Some("src/a.rs")); + assert_eq!(got[0].new_position, Some(12)); + assert_eq!(got[0].old_position, Some(0)); + assert_eq!(got[0].body.as_deref(), Some("off by one")); + assert_eq!( + got[1].path.as_deref(), + Some("docs/x:y.md"), + "the last colon splits" + ); + assert_eq!(got[1].new_position, Some(3)); + } + + #[test] + fn a_malformed_at_is_refused_with_the_reason() { + for (pairs, needle) in [ + (at(&[("src/a.rs", "no line")]), "expected PATH:LINE"), + (at(&[("src/a.rs:x", "nan")]), "positive number"), + (at(&[("src/a.rs:0", "zero")]), "positive number"), + (at(&[("src/a.rs:-4", "negative")]), "positive number"), + (at(&[(":5", "no path")]), "PATH is empty"), + (at(&[("src/a.rs:5", " ")]), "text is empty"), + (vec!["src/a.rs:5".to_owned()], "PATH:LINE and a TEXT"), + ] { + let err = format!("{:#}", parse_inline_comments(&pairs).unwrap_err()); + assert!(err.contains(needle), "{pairs:?} -> {err}"); + } + } + + #[test] + fn no_at_means_no_inline_comments() { + assert!(parse_inline_comments(&[]).unwrap().is_empty()); + } + fn comment(id: i64, position: u64, resolver: Option<&str>) -> Value { json!({ "id": id,