forge cli: unresolved filter drops only resolved comments, position-less ones stay labelled

hive-forge/src/verbs/pr_reviews.rs

@@ -34,9 +34,8 @@ pub struct Args {
     body: Option<String>,
 
     /// When listing: narrow the inline comments shown (and leave out the
-    /// reviews with none left). `unresolved` = still open on the current
-    /// head, that is neither resolved nor outdated: what a re-review has to
-    /// check.
+    /// reviews with none left). `unresolved` keeps every comment nobody has
+    /// resolved yet: what a re-review has to check.
     #[arg(long, value_enum, conflicts_with_all = ["approve", "request_changes", "comment"])]
     only: Option<Only>,
 }
@@ -44,7 +43,9 @@ pub struct Args {
 /// Which inline comments `--only` keeps.
 #[derive(Clone, Copy, PartialEq, Eq, clap::ValueEnum)]
 enum Only {
-    /// Neither resolved nor outdated.
+    /// Not resolved yet. Comments without a position stay in: one might sit on
+    /// a line a push moved or on the whole file, and hiding it could hide a
+    /// live finding.
     Unresolved,
 }
 
@@ -150,9 +151,10 @@ pub(crate) fn comment_location(c: &Value) -> String {
     }
 }
 
-/// An inline comment whose anchored line is no longer in the diff (a push
-/// moved it): Forgejo drops `position` then.
-fn is_outdated(c: &Value) -> bool {
+/// An inline comment with no line position. Forgejo drops `position` when a
+/// push moved the anchored line out of the diff, but a file-level comment or
+/// one on a removed line may lack it too, so this is a hint, not a verdict.
+fn has_no_position(c: &Value) -> bool {
     c.get("position")
         .and_then(Value::as_u64)
         .is_none_or(|l| l == 0)
@@ -165,13 +167,15 @@ fn resolved_by(c: &Value) -> Option<&str> {
         .and_then(Value::as_str)
 }
 
-/// Still needs an answer on the current head: neither resolved nor outdated.
-fn is_open(c: &Value) -> bool {
-    resolved_by(c).is_none() && !is_outdated(c)
+/// Nobody has resolved the conversation yet. Position plays no part: a
+/// comment without one may be a live finding, and a filter built for a
+/// re-review gate must not hide it.
+fn is_unresolved(c: &Value) -> bool {
+    resolved_by(c).is_none()
 }
 
 /// One inline comment as text: `[#id path:line] body`, continuation lines
-/// indented, `(resolved by X)` / `(outdated)` after the body. Shared by
+/// indented, `(resolved by X)` / `(no position: ...)` after the body. Shared by
 /// `pr reviews` and `pr timeline`.
 pub(crate) fn render_inline_comment(c: &Value) -> String {
     let id = c.get("id").and_then(Value::as_i64).unwrap_or(0);
@@ -185,8 +189,8 @@ pub(crate) fn render_inline_comment(c: &Value) -> String {
     if let Some(who) = resolved_by(c) {
         write!(out, " (resolved by {who})").ok();
     }
-    if is_outdated(c) {
-        out.push_str(" (outdated)");
+    if has_no_position(c) {
+        out.push_str(" (no position: outdated or file-level)");
     }
     out
 }
@@ -195,7 +199,7 @@ pub(crate) fn render_inline_comment(c: &Value) -> String {
 /// the matching ones.
 fn visible(comments: Vec<Value>, only: Option<Only>) -> Vec<Value> {
     match only {
-        Some(Only::Unresolved) => comments.into_iter().filter(is_open).collect(),
+        Some(Only::Unresolved) => comments.into_iter().filter(is_unresolved).collect(),
         None => comments,
     }
 }
@@ -229,7 +233,7 @@ fn list_reviews_json(
                         "line": c.get("position"),
                         "body": c.get("body"),
                         "resolved_by": resolved_by(c),
-                        "outdated": is_outdated(c),
+                        "no_position": has_no_position(c),
                     })
                 })
                 .collect();
@@ -335,14 +339,15 @@ mod tests {
     }
 
     #[test]
-    fn open_means_neither_resolved_nor_outdated() {
-        assert!(is_open(&comment(1, 42, None)));
-        assert!(!is_open(&comment(1, 42, Some("mara"))));
-        assert!(!is_open(&comment(1, 0, None)));
+    fn unresolved_means_only_that_nobody_resolved_it() {
+        assert!(is_unresolved(&comment(1, 42, None)));
+        assert!(!is_unresolved(&comment(1, 42, Some("mara"))));
+        // No position does not hide a comment: it may be a live finding.
+        assert!(is_unresolved(&comment(1, 0, None)));
     }
 
     #[test]
-    fn unresolved_filter_keeps_only_open_comments() {
+    fn unresolved_filter_drops_only_resolved_comments() {
         let all = vec![
             comment(1, 42, None),
             comment(2, 42, Some("mara")),
@@ -350,7 +355,10 @@ mod tests {
         ];
         let ids =
             |v: Vec<Value>| -> Vec<i64> { v.iter().filter_map(|c| c["id"].as_i64()).collect() };
-        assert_eq!(ids(visible(all.clone(), Some(Only::Unresolved))), vec![1]);
+        assert_eq!(
+            ids(visible(all.clone(), Some(Only::Unresolved))),
+            vec![1, 3]
+        );
         assert_eq!(ids(visible(all, None)), vec![1, 2, 3]);
     }
 
@@ -366,7 +374,7 @@ mod tests {
         );
         assert_eq!(
             render_inline_comment(&comment(7, 0, None)),
-            "[#7 src/a.rs] off by one (outdated)"
+            "[#7 src/a.rs] off by one (no position: outdated or file-level)"
         );
     }