feat: anchor review comments to precise text ranges

src/review.rs

@@ -10,6 +10,7 @@ use std::{
         atomic::{AtomicU64, Ordering},
     },
 };
+use unicode_segmentation::UnicodeSegmentation;
 
 const LIMIT: usize = 64 * 1024 * 1024;
 static NEXT_TEMP: AtomicU64 = AtomicU64::new(0);
@@ -43,13 +44,19 @@ pub struct Anchor {
     pub revision: String,
     pub path: PathBuf,
     pub side: Side,
-    pub start_line: usize,
-    pub end_line: usize,
+    /// Zero-based extended-grapheme bounds; `end` is exclusive.
+    pub start: crate::plugin::TextPosition,
+    pub end: crate::plugin::TextPosition,
     /// Exact whole-side text, including its final newline when present.
     // ponytail: whole-file text per comment; deduplicate snapshots if the 64 MiB ceiling matters.
     pub content: String,
 }
 impl Anchor {
+    pub fn rows(&self) -> std::ops::RangeInclusive<usize> {
+        let end = self.end.row - usize::from(self.end.column == 0 && self.end.row > self.start.row);
+        self.start.row..=end
+    }
+
     pub fn matches(&self, revision: &str, path: &Path, side: Side, text: &str) -> bool {
         self.revision == revision && self.path == path && self.side == side && self.content == text
     }
@@ -91,6 +98,91 @@ pub struct Review {
     #[serde(skip)]
     saved: RefCell<Option<(PathBuf, Arc<Vec<u8>>)>>,
 }
+
+#[derive(Deserialize)]
+#[serde(deny_unknown_fields)]
+struct LegacyReview {
+    version: u32,
+    root: PathBuf,
+    summary: String,
+    verdict: Option<Verdict>,
+    comments: Vec<LegacyComment>,
+}
+
+#[derive(Deserialize)]
+#[serde(deny_unknown_fields)]
+struct LegacyComment {
+    id: u64,
+    anchor: LegacyAnchor,
+    body: String,
+}
+
+#[derive(Deserialize)]
+#[serde(deny_unknown_fields)]
+struct LegacyAnchor {
+    revision: String,
+    path: PathBuf,
+    side: Side,
+    start_line: usize,
+    end_line: usize,
+    content: String,
+}
+
+impl LegacyReview {
+    fn migrate(self) -> Result<Review, String> {
+        if self.version != 1 {
+            return Err(format!("Unsupported review version {}", self.version));
+        }
+        let comments = self
+            .comments
+            .into_iter()
+            .map(|comment| {
+                let old = comment.anchor;
+                if old.start_line == 0 || old.end_line < old.start_line {
+                    return Err("Legacy review ranges must be inclusive and nonzero".into());
+                }
+                let mut lines = old.content.split('\n');
+                let last = lines
+                    .nth(old.end_line - 1)
+                    .ok_or("Legacy review range is outside the anchor text")?;
+                let end = if lines.next().is_some() {
+                    crate::plugin::TextPosition {
+                        row: old.end_line,
+                        column: 0,
+                    }
+                } else {
+                    crate::plugin::TextPosition {
+                        row: old.end_line - 1,
+                        column: last.graphemes(true).count(),
+                    }
+                };
+                Ok(Comment {
+                    id: comment.id,
+                    anchor: Anchor {
+                        revision: old.revision,
+                        path: old.path,
+                        side: old.side,
+                        start: crate::plugin::TextPosition {
+                            row: old.start_line - 1,
+                            column: 0,
+                        },
+                        end,
+                        content: old.content,
+                    },
+                    body: comment.body,
+                })
+            })
+            .collect::<Result<_, String>>()?;
+        Ok(Review {
+            version: 2,
+            root: self.root,
+            summary: self.summary,
+            verdict: self.verdict,
+            comments,
+            saved: RefCell::new(None),
+        })
+    }
+}
 impl Default for Review {
     fn default() -> Self {
         Self::new(PathBuf::new())
@@ -114,7 +206,7 @@ impl Review {
             fs::canonicalize(&root).unwrap_or(root)
         };
         Self {
-            version: 1,
+            version: 2,
             root,
             summary: String::new(),
             verdict: None,
@@ -124,7 +216,7 @@ impl Review {
     }
 
     fn validate(&self) -> Result<(), String> {
-        if self.version != 1 {
+        if self.version != 2 {
             return Err(format!("Unsupported review version {}", self.version));
         }
         if !self.root.is_absolute() || !self.root.is_dir() {
@@ -143,13 +235,28 @@ impl Review {
             if anchor.revision.is_empty() || anchor.revision.chars().any(char::is_control) {
                 return Err("Review anchors require an immutable revision identity".into());
             }
-            if anchor.start_line == 0
-                || anchor.end_line < anchor.start_line
-                || anchor.end_line > anchor.content.split('\n').count()
-            {
-                return Err(
-                    "Review ranges must be inclusive, nonzero and within the anchor text".into(),
-                );
+            if anchor.start > anchor.end {
+                return Err("Review ranges must be ordered and end-exclusive".into());
+            }
+            let mut end_exists = false;
+            for (row, line) in anchor.content.split('\n').enumerate() {
+                if row == anchor.start.row || row == anchor.end.row {
+                    let columns = line.graphemes(true).count();
+                    if (row == anchor.start.row && anchor.start.column > columns)
+                        || (row == anchor.end.row && anchor.end.column > columns)
+                    {
+                        return Err(
+                            "Review columns must be within the anchor's grapheme text".into()
+                        );
+                    }
+                }
+                if row == anchor.end.row {
+                    end_exists = true;
+                    break;
+                }
+            }
+            if !end_exists {
+                return Err("Review rows must be within the anchor text".into());
             }
         }
         Ok(())
@@ -157,8 +264,20 @@ impl Review {
 
     pub fn load(path: &Path, root: &Path) -> Result<Self, String> {
         let bytes = read_file(path)?.ok_or("Review file does not exist")?;
-        let mut review: Self = serde_json::from_slice(&bytes)
+        #[derive(Deserialize)]
+        struct Version {
+            version: u32,
+        }
+        let Version { version } = serde_json::from_slice(&bytes)
             .map_err(|error| format!("Invalid review JSON: {error}"))?;
+        let mut review: Self = match version {
+            1 => serde_json::from_slice::<LegacyReview>(&bytes)
+                .map_err(|error| format!("Invalid review JSON: {error}"))?
+                .migrate()?,
+            2 => serde_json::from_slice(&bytes)
+                .map_err(|error| format!("Invalid review JSON: {error}"))?,
+            _ => return Err(format!("Unsupported review version {version}")),
+        };
         review.validate()?;
         let expected = fs::canonicalize(root).map_err(|error| error.to_string())?;
         let actual = fs::canonicalize(&review.root).map_err(|error| error.to_string())?;
@@ -278,14 +397,19 @@ mod tests {
         }
     }
 
-    #[test]
-    fn persistence_validates_anchors_and_preserves_files_on_failure() {
+    fn directory() -> Directory {
         let directory = Directory(std::env::temp_dir().join(format!(
             "vamp-review-test-{}-{}",
             std::process::id(),
             NEXT_TEMP.fetch_add(1, Ordering::Relaxed),
         )));
         fs::create_dir(&directory.0).unwrap();
+        directory
+    }
+
+    #[test]
+    fn persistence_validates_anchors_and_preserves_files_on_failure() {
+        let directory = directory();
         let root = directory.0.canonicalize().unwrap();
         let path = root.join("review.json");
         let mut review = Review::new(root.clone());
@@ -297,8 +421,8 @@ mod tests {
                 revision: "working:immutable".into(),
                 path: "source.rs".into(),
                 side: Side::After,
-                start_line: 1,
-                end_line: 2,
+                start: crate::plugin::TextPosition { row: 0, column: 0 },
+                end: crate::plugin::TextPosition { row: 2, column: 0 },
                 content: "first\nlast\n".into(),
             },
             body: "Please adjust this".into(),
@@ -340,7 +464,7 @@ mod tests {
         assert!(Review::new(root.clone()).save(&path).is_err());
         let valid = fs::read(&path).unwrap();
         for (field, value) in [
-            ("version", serde_json::json!(2)),
+            ("version", serde_json::json!(3)),
             ("root", serde_json::json!(root.join("other"))),
             (
                 "comments",
@@ -356,9 +480,9 @@ mod tests {
             ("path", serde_json::json!("../outside")),
             ("path", serde_json::json!("/absolute")),
             ("path", serde_json::json!(".git/config")),
-            ("start_line", serde_json::json!(0)),
-            ("start_line", serde_json::json!(3)),
-            ("end_line", serde_json::json!(4)),
+            ("start", serde_json::json!({"row": 2, "column": 1})),
+            ("start", serde_json::json!({"row": 3, "column": 0})),
+            ("end", serde_json::json!({"row": 3, "column": 0})),
             ("revision", serde_json::json!("")),
         ] {
             let mut invalid = serde_json::to_value(&loaded).unwrap();
@@ -374,7 +498,7 @@ mod tests {
         assert_eq!(fs::read(&path).unwrap(), b"external edit");
         assert_eq!(loaded.summary, "Updated");
         let invalid = root.join("invalid.json");
-        loaded.comments[0].anchor.start_line = 0;
+        loaded.comments[0].anchor.start.row = usize::MAX;
         assert!(loaded.save(&invalid).is_err());
         assert!(!invalid.exists());
         let large = root.join("large.json");
@@ -392,4 +516,149 @@ mod tests {
             assert_eq!(fs::read(&path).unwrap(), b"external edit");
         }
     }
+
+    #[test]
+    fn legacy_lines_migrate_losslessly_and_save_exact_v2_ranges() {
+        use crate::plugin::TextPosition;
+        let directory = directory();
+        let root = directory.0.canonicalize().unwrap();
+        let path = root.join("review.json");
+        let cases = [
+            ("first\nlast\n", 1, 2, (0, 0), (2, 0), 0..=1),
+            ("first\nlast", 2, 2, (1, 0), (1, 4), 1..=1),
+            ("é👩‍🔬e\u{301}", 1, 1, (0, 0), (0, 3), 0..=0),
+            ("first\n\n", 2, 2, (1, 0), (2, 0), 1..=1),
+            ("first\n", 2, 2, (1, 0), (1, 0), 1..=1),
+            ("", 1, 1, (0, 0), (0, 0), 0..=0),
+        ];
+        let comments: Vec<_> = cases
+            .iter()
+            .enumerate()
+            .map(|(index, case)| {
+                serde_json::json!({
+                    "id": index + 11,
+                    "anchor": {
+                        "revision": "base:immutable",
+                        "path": "old/name.rs",
+                        "side": "before",
+                        "start_line": case.1,
+                        "end_line": case.2,
+                        "content": case.0,
+                    },
+                    "body": format!("Comment {index}"),
+                })
+            })
+            .collect();
+        let original = serde_json::json!({
+            "version": 1,
+            "root": root,
+            "summary": "Keep this summary",
+            "verdict": "looks_good",
+            "comments": comments,
+        });
+        fs::write(&path, serde_json::to_vec(&original).unwrap()).unwrap();
+        let migrated = Review::load(&path, &root).unwrap();
+        assert_eq!(migrated.version, 2);
+        assert_eq!(migrated.root, root);
+        assert_eq!(migrated.summary, "Keep this summary");
+        assert_eq!(migrated.verdict, Some(Verdict::LooksGood));
+        for (index, (comment, case)) in migrated.comments.iter().zip(&cases).enumerate() {
+            assert_eq!(comment.id, index as u64 + 11);
+            assert_eq!(comment.body, format!("Comment {index}"));
+            assert_eq!(comment.anchor.revision, "base:immutable");
+            assert_eq!(comment.anchor.path, Path::new("old/name.rs"));
+            assert_eq!(comment.anchor.side, Side::Before);
+            assert_eq!(comment.anchor.content, case.0);
+            assert_eq!(
+                comment.anchor.start,
+                TextPosition {
+                    row: case.3.0,
+                    column: case.3.1
+                }
+            );
+            assert_eq!(
+                comment.anchor.end,
+                TextPosition {
+                    row: case.4.0,
+                    column: case.4.1
+                }
+            );
+            assert_eq!(comment.anchor.rows(), case.5);
+        }
+        migrated.save(&path).unwrap();
+        let saved: serde_json::Value = serde_json::from_slice(&fs::read(&path).unwrap()).unwrap();
+        assert_eq!(saved["version"], 2);
+        assert!(saved["comments"][0]["anchor"].get("start_line").is_none());
+        assert!(saved["comments"][0]["anchor"].get("end_line").is_none());
+        assert_eq!(
+            Review::load(&path, &root).unwrap().comments,
+            migrated.comments
+        );
+
+        for (start, end) in [(0, 1), (2, 1), (1, 4)] {
+            let mut invalid = original.clone();
+            invalid["comments"][0]["anchor"]["start_line"] = start.into();
+            invalid["comments"][0]["anchor"]["end_line"] = end.into();
+            fs::write(&path, serde_json::to_vec(&invalid).unwrap()).unwrap();
+            assert!(Review::load(&path, &root).is_err());
+        }
+        let mut mixed = original.clone();
+        mixed["comments"][0]["anchor"]["start"] = serde_json::json!({"row": 0, "column": 0});
+        fs::write(&path, serde_json::to_vec(&mixed).unwrap()).unwrap();
+        assert!(Review::load(&path, &root).is_err());
+        let mut wrong_version = original;
+        wrong_version["version"] = 2.into();
+        fs::write(&path, serde_json::to_vec(&wrong_version).unwrap()).unwrap();
+        assert!(Review::load(&path, &root).is_err());
+    }
+
+    #[test]
+    fn exact_grapheme_ranges_roundtrip_and_reject_invalid_coordinates() {
+        use crate::plugin::TextPosition;
+        let directory = directory();
+        let root = directory.0.canonicalize().unwrap();
+        let path = root.join("review.json");
+        let mut review = Review::new(root.clone());
+        review.comments.push(Comment {
+            id: 1,
+            anchor: Anchor {
+                revision: "working:immutable".into(),
+                path: "source.rs".into(),
+                side: Side::After,
+                start: TextPosition { row: 0, column: 1 },
+                end: TextPosition { row: 1, column: 1 },
+                content: "a👩‍🔬e\u{301}\n界x\n".into(),
+            },
+            body: "Exact text".into(),
+        });
+        review.save(&path).unwrap();
+        let mut loaded = Review::load(&path, &root).unwrap();
+        assert_eq!(loaded.comments, review.comments);
+        assert_eq!(loaded.comments[0].anchor.rows(), 0..=1);
+        let valid = serde_json::to_value(&review).unwrap();
+        for (field, value) in [
+            ("start", serde_json::json!({"row": 0, "column": 4})),
+            ("end", serde_json::json!({"row": 1, "column": 3})),
+            ("end", serde_json::json!({"row": 2, "column": 1})),
+            ("end", serde_json::json!({"row": 3, "column": 0})),
+            ("end", serde_json::json!({"row": 0, "column": 0})),
+            ("start", serde_json::json!({"row": -1, "column": 0})),
+            (
+                "start",
+                serde_json::json!({"row": 0, "column": 0, "extra": 0}),
+            ),
+            ("start_line", serde_json::json!(1)),
+        ] {
+            let mut invalid = valid.clone();
+            invalid["comments"][0]["anchor"][field] = value;
+            fs::write(&path, serde_json::to_vec(&invalid).unwrap()).unwrap();
+            assert!(Review::load(&path, &root).is_err(), "{field}");
+        }
+        let mut wrong_version = valid;
+        wrong_version["version"] = 1.into();
+        fs::write(&path, serde_json::to_vec(&wrong_version).unwrap()).unwrap();
+        assert!(Review::load(&path, &root).is_err());
+        loaded.comments[0].anchor.end = TextPosition { row: 0, column: 0 };
+        assert!(loaded.save(&root.join("backwards.json")).is_err());
+    }
 }