refactor: normalize review and comment storage

docs/architecture.md

@@ -17,8 +17,8 @@ One editor crate; the workspace also includes its patched Crossterm dependency.
   Virtual diff leaves retain their original catalog input index for caller-owned annotations. Normal scanned leaves accept optional repository status and old-path metadata without filtering unchanged files; missing tracked deletions add virtual leaves and required directories. Metadata-only updates preserve query, selection, expansion, and view without rebuilding the catalog.
 - `diff`: owns searchable Before/After selection, category, focus and explorer visibility, request freshness, read-only comparison DTOs, aligned rows with cached grapheme-level change ranges, and shared vertical/independent horizontal offsets. Changed paths form a virtual `files::FilePicker` catalog, reusing the normal tree projection, filtering, and navigation without scanning disk. It has no subprocess or terminal dependencies.
 - `repository`: discovers jj/Git repositories and local/advertised remote revisions, builds disk snapshots and changed-file hunks, and owns isolated temporary comparison repositories with bounded, cancellable subprocesses. It never saves editor buffers or modifies active refs/checkouts.
-- `review`: owns the validated local review model, exact immutable anchors, SQLite storage through bundled rusqlite, and atomic JSON import/export with external-change protection. `Store` uses schema-versioned transactions and compare-and-swap review revisions; repository roots and persistent review IDs are independent of comparison revisions. One bounded JSON document is stored per row, retaining exact grapheme endpoints and stale snapshots. Incompatible schemas and concurrent edits fail without replacing data or advancing in-memory saved state.
-  Store listing returns validated review documents in descending ID order. CLI metadata, sole-review restoration, and selector labels reuse those documents without reloading them; selector choices live only in the displayed buffer.
+- `review`: owns the validated local review model, exact immutable anchors, SQLite storage through bundled rusqlite, and atomic JSON import/export with external-change protection. Schema 2 stores review identity, repository, revision, summary, and verdict in `reviews`; `comments` stores ordered comment IDs, bodies, resolution states, and explicit immutable anchor fields. Foreign keys, unique keys, and checks enforce relationships and scalar invariants. Full `u64` comment IDs use checked decimal text because SQLite integers are signed. No JSON documents are stored in SQLite.
+  `Store` retains compare-and-swap review revisions and atomic whole-review saves: metadata and replacement comment rows commit together. Read transactions reconstruct validated reviews from one consistent snapshot, including when listing in descending ID order. Stored text is bounded to 64 MiB per review, including a fixed per-comment overhead. JSON version 3 is an independent import/export format. Incompatible databases, including schema 1 document storage, are rejected and preserved without migration or reset. CLI metadata, sole-review restoration, and selector labels reuse loaded reviews; selector choices live only in the displayed buffer.
   The review CLI keeps `list`/`show` read-only. Comment deletion validates the explicit review/comment IDs through an existing read-only store before opening it for a compare-and-swap save; absent storage and invalid targets do not create reviews or a database.
   `editor_review` commits native comment/summary buffers on dismissal or switching, and verdicts/deletions immediately. It restores a sole stored review or opens the floating selector for multiple reviews. Exports retain their own file baseline and do not become automatic persistence targets; imports get fresh database identities. Review editors remain tab-owned native panes outside the workspace `PaneTree`. Storage opens lazily when a comparison is ready; the session supplies the configured or XDG default directory. Direct `Editor` callers inject a directory with `set_review_directory` to enable persistence.
   Successful SQLite autosaves and JSON-only saves share one acknowledgement operation: clear review dirtiness and advance the owning tab's native draft saved baselines only after persistence succeeds. Failed writes retain unsaved state; JSON exports do not acknowledge a pending SQLite save.

docs/development.md

@@ -70,11 +70,12 @@ Diff checks cover staged category/filter navigation, stale request cancellation,
 
 Local review checks cover native comment/summary composition and undo, Visual ranges on either side, verdicts, JSON save/reopen, external-write refusal, hidden drafts and teardown guards, immutable Working identities in Git/jj, renamed paths, live EOF, and stale anchors. Render checks cover selected/range markers, split/unified context, wrapped continuations, and narrow gutters. Live PTY smoke creates and saves a comment/summary/verdict, closes and reopens the review, edits the reopened comment, changes the reviewed source, and observes stale status without altering the saved anchor. Fixtures are local repositories; no forge writes.
 
-SQLite review checks cover exact anchors across restarts and revision changes, repository isolation, independent connections and optimistic conflicts, locked/unwritable stores, incompatible schema preservation, bounded/corrupt documents, private file permissions, and XDG/config path resolution. Editor checks require explicit writes for comment/summary text, discard unsaved text on dismissal without retaining discarded undo history, prevent metadata autosaves from committing hidden drafts, persist verdicts/deletions, restore a sole review, choose among multiple reviews, and keep exports independent from autosaves. Test fixtures use isolated review directories. Live PTY smoke verifies that Escape leaves no new stored comment, `:w` persists exact text, and pane close discards a later edit; reopening and writing again preserves the saved body.
+SQLite review checks cover exact anchors across restarts and revision changes, repository isolation, independent connections and optimistic conflicts, locked/unwritable stores, incompatible schema preservation, relational constraints, full unsigned comment IDs, stable comment order, bounded/corrupt rows, rollback after partial comment insertion, private file permissions, and XDG/config path resolution. Schema 1 document storage is rejected without modification. Editor checks require explicit writes for comment/summary text, discard unsaved text on dismissal without retaining discarded undo history, prevent metadata autosaves from committing hidden drafts, persist verdicts/deletions, restore a sole review, choose among multiple reviews, and keep exports independent from autosaves. Test fixtures use isolated review directories. Live PTY smoke verifies that Escape leaves no new stored comment, `:w` persists exact text, and pane close discards a later edit; reopening and writing again preserves the saved body.
 
 Manual-resolution checks cover immediate persistence, reopen/export/import, independent stale status, conflict-retained drafts, and incompatible version preservation. CLI checks cover filename-safe dispatch, Unicode excerpts, unresolved filtering/counts, absent storage, read-only access and permissions, symlink rejection, and incompatible schema preservation.
 Live PTY/CLI smoke composes and explicitly saves a comment, resolves it in the editor, observes an empty default CLI comment list with explicit counts, retrieves the resolved comment and exact excerpt with `--all`, and unresolves it. Each transition advances the same stored review revision; listing absent storage creates no data directory.
 CLI deletion checks exercise explicit review/comment IDs, resolved-comment deletion, repository isolation, preserved summaries/verdicts/other comments, last-comment deletion, missing targets/storage, and stale-save conflicts through the executable. CLI smoke verifies persisted deletion and rejects a repeated deletion without advancing the review revision.
+Relational-storage smoke inspects typed review/comment columns, round-trips exact Unicode anchors through the store and JSON export/import, deletes through the real CLI, and confirms the stored review and existing export remain intact.
 
 
 Conflicted-revision regressions preserve unchanged symlinks, defer oversized unselected blobs until file selection, compare selected conflict text with jj's projection, and exclude unchanged conflicts when comparing later revisions. Live terminal smoke covers local bookmark selection and opening an actual conflicted file without changing the checkout.

docs/editing.md

@@ -493,7 +493,9 @@ Storage defaults to `$XDG_DATA_HOME/vamp/reviews/reviews.sqlite3`, or `~/.local/
 
 Write errors, locked databases, incompatible schemas, and conflicting edits from another editor instance retain dirty drafts and report the failure. Failed text saves leave the committed review unchanged; unrelated metadata autosaves cannot commit those drafts. Normal destructive closes remain guarded until drafts are explicitly saved or discarded. `:w!` does not bypass database revision conflicts; export your draft before reopening the current stored review. JSON exports retain external-file-change protection. Only the current database schema is accepted; existing incompatible databases are preserved rather than reset or migrated.
 
-Version 3 JSON is the agent-readable sharing format: repository root, summary, optional verdict, and comments with stable IDs, bodies, required boolean `resolved` states, and anchors. New comments are unresolved. Database review IDs and optimistic storage revisions stay internal. Each anchor retains its immutable comparison revision, side, relative path (old path for renamed Before files), and exact whole-file text including the final newline. `start` and `end` contain zero-based source `row` and extended-grapheme `column` coordinates; `end` is exclusive. Columns count complete Unicode graphemes, not bytes, code points, UTF-16 units, or screen cells. Overview/window labels display one-based rows and columns. Older formats, including version 2, are rejected and preserved without migration. Working comparisons use content-addressed snapshot IDs; captured text additionally freezes live unsaved edits. Each stored or exported review is limited to 64 MiB.
+The current SQLite format is schema 2: typed `reviews` and `comments` tables, not serialized JSON documents. Schema 1 databases are incompatible and are left untouched; there is no migration or automatic reset. JSON version 3 remains the sharing format, independent of the database schema.
+
+Version 3 JSON is the agent-readable sharing format: repository root, summary, optional verdict, and comments with stable IDs, bodies, required boolean `resolved` states, and anchors. New comments are unresolved. Database review IDs and optimistic storage revisions stay internal. Each anchor retains its immutable comparison revision, side, relative path (old path for renamed Before files), and exact whole-file text including the final newline. `start` and `end` contain zero-based source `row` and extended-grapheme `column` coordinates; `end` is exclusive. Columns count complete Unicode graphemes, not bytes, code points, UTF-16 units, or screen cells. Overview/window labels display one-based rows and columns. Older JSON formats, including version 2, are rejected and preserved without migration. Working comparisons use content-addressed snapshot IDs; captured text additionally freezes live unsaved edits. Stored reviews have a 64 MiB aggregate text budget including 64 bytes of overhead per comment; imported/exported JSON files have a separate 64 MiB serialized-size limit.
 
 Reopening requires the same repository root. Comments whose revision, path, or exact file text no longer matches are **stale**: their original anchors remain unchanged and their gutter markers disappear. Unloaded files are shown as **not checked** until visited; refresh the overview to update its statuses. There is no automatic reanchoring, server, authentication, or review-publishing operation.
 

src/review.rs

@@ -281,12 +281,36 @@ fn directory_from(xdg: Option<&OsStr>, home: Option<&OsStr>) -> Result<PathBuf,
         .ok_or_else(|| "Cannot locate review storage: HOME must be an absolute directory".into())
 }
 
-// ponytail: whole-review JSON commits; normalize comment rows if large reviews make writes costly.
-const SCHEMA: &str = "CREATE TABLE reviews (
+const REVIEWS_SCHEMA: &str = "CREATE TABLE reviews (
     id INTEGER PRIMARY KEY CHECK (id > 0),
     root TEXT NOT NULL,
     revision INTEGER NOT NULL CHECK (revision > 0),
-    document TEXT NOT NULL CHECK (length(CAST(document AS BLOB)) <= 67108864)
+    summary TEXT NOT NULL,
+    verdict TEXT CHECK (verdict IN ('looks_good', 'changes_requested'))
+) STRICT";
+
+// SQLite integers are signed; decimal text preserves the model's full u64 comment IDs.
+const COMMENTS_SCHEMA: &str = "CREATE TABLE comments (
+    review_id INTEGER NOT NULL REFERENCES reviews(id) ON DELETE CASCADE,
+    id TEXT NOT NULL CHECK (
+        length(id) BETWEEN 1 AND 20 AND id NOT GLOB '*[^0-9]*'
+        AND substr(id, 1, 1) BETWEEN '1' AND '9'
+        AND (length(id) < 20 OR id <= '18446744073709551615')
+    ),
+    position INTEGER NOT NULL CHECK (position >= 0),
+    body TEXT NOT NULL,
+    resolved INTEGER NOT NULL CHECK (resolved IN (0, 1)),
+    anchor_revision TEXT NOT NULL CHECK (length(anchor_revision) > 0),
+    path TEXT NOT NULL,
+    side TEXT NOT NULL CHECK (side IN ('before', 'after')),
+    start_row INTEGER NOT NULL CHECK (start_row >= 0),
+    start_column INTEGER NOT NULL CHECK (start_column >= 0),
+    end_row INTEGER NOT NULL CHECK (end_row >= 0),
+    end_column INTEGER NOT NULL CHECK (end_column >= 0),
+    content TEXT NOT NULL,
+    PRIMARY KEY (review_id, id),
+    UNIQUE (review_id, position),
+    CHECK (end_row > start_row OR (end_row = start_row AND end_column >= start_column))
 ) STRICT";
 
 pub struct Store {
@@ -316,6 +340,9 @@ impl Store {
         )
         .map_err(database_error)?;
         Self::check_schema(&connection)?;
+        connection
+            .pragma_update(None, "foreign_keys", true)
+            .map_err(database_error)?;
         Ok(Some(Self { connection }))
     }
 
@@ -330,7 +357,7 @@ impl Store {
             .map_err(database_error)?
             .collect::<Result<_, _>>()
             .map_err(database_error)?;
-        if version != 1 || schema != [SCHEMA] {
+        if version != 2 || schema != [COMMENTS_SCHEMA, REVIEWS_SCHEMA] {
             return Err(format!(
                 "Unsupported review database schema (version {version}); existing data was not changed"
             ));
@@ -378,6 +405,9 @@ impl Store {
                 | OpenFlags::SQLITE_OPEN_NOFOLLOW,
         )
         .map_err(database_error)?;
+        connection
+            .pragma_update(None, "foreign_keys", true)
+            .map_err(database_error)?;
         connection
             .busy_timeout(std::time::Duration::ZERO)
             .map_err(database_error)?;
@@ -385,9 +415,14 @@ impl Store {
             let transaction = connection
                 .transaction_with_behavior(TransactionBehavior::Immediate)
                 .map_err(database_error)?;
-            transaction.execute_batch(SCHEMA).map_err(database_error)?;
             transaction
-                .pragma_update(None, "user_version", 1)
+                .execute_batch(REVIEWS_SCHEMA)
+                .map_err(database_error)?;
+            transaction
+                .execute_batch(COMMENTS_SCHEMA)
+                .map_err(database_error)?;
+            transaction
+                .pragma_update(None, "user_version", 2)
                 .map_err(database_error)?;
             transaction.commit().map_err(database_error)?;
         } else {
@@ -407,17 +442,24 @@ impl Store {
         let root_text = root
             .to_str()
             .ok_or("Review repository root must be UTF-8")?;
-        let mut statement = self
+        let transaction = self
             .connection
-            .prepare(
-                "SELECT id, root, revision, document FROM reviews WHERE root = ?1 ORDER BY id DESC",
-            )
+            .unchecked_transaction()
             .map_err(database_error)?;
-        let mut rows = statement.query([root_text]).map_err(database_error)?;
-        let mut reviews = Vec::new();
-        while let Some(row) = rows.next().map_err(database_error)? {
-            reviews.push(stored_review(row, &root)?);
-        }
+        let reviews = {
+            let mut statement = transaction
+                .prepare(
+                    "SELECT id, root, revision, summary, verdict FROM reviews WHERE root = ?1 ORDER BY id DESC",
+                )
+                .map_err(database_error)?;
+            let mut rows = statement.query([root_text]).map_err(database_error)?;
+            let mut reviews = Vec::new();
+            while let Some(row) = rows.next().map_err(database_error)? {
+                reviews.push(stored_review(&transaction, row, &root)?);
+            }
+            reviews
+        };
+        transaction.commit().map_err(database_error)?;
         Ok(reviews)
     }
 
@@ -426,18 +468,25 @@ impl Store {
         let root_text = root
             .to_str()
             .ok_or("Review repository root must be UTF-8")?;
-        let mut statement = self
+        let transaction = self
             .connection
-            .prepare("SELECT id, root, revision, document FROM reviews WHERE id = ?1 AND root = ?2")
+            .unchecked_transaction()
             .map_err(database_error)?;
-        let mut rows = statement
-            .query(params![id, root_text])
-            .map_err(database_error)?;
-        let row = rows
-            .next()
-            .map_err(database_error)?
-            .ok_or("Review does not exist in this repository")?;
-        stored_review(row, &root)
+        let review = {
+            let mut statement = transaction
+                .prepare("SELECT id, root, revision, summary, verdict FROM reviews WHERE id = ?1 AND root = ?2")
+                .map_err(database_error)?;
+            let mut rows = statement
+                .query(params![id, root_text])
+                .map_err(database_error)?;
+            let row = rows
+                .next()
+                .map_err(database_error)?
+                .ok_or("Review does not exist in this repository")?;
+            stored_review(&transaction, row, &root)?
+        };
+        transaction.commit().map_err(database_error)?;
+        Ok(review)
     }
 
     pub fn save(&mut self, review: &mut Review) -> Result<(), String> {
@@ -457,19 +506,34 @@ impl Store {
             None if review.storage_revision == 0 => 1,
             _ => return Err("Invalid review storage identity".into()),
         };
-        let bytes = serde_json::to_vec(review).map_err(|error| error.to_string())?;
-        if bytes.len() > LIMIT {
+        let mut size = review.root.as_os_str().len() + review.summary.len();
+        for comment in &review.comments {
+            size = size
+                .checked_add(
+                    comment.body.len()
+                        + comment.anchor.revision.len()
+                        + comment.anchor.path.as_os_str().len()
+                        + comment.anchor.content.len()
+                        + 64,
+                )
+                .ok_or("Review exceeds the 64 MiB limit")?;
+        }
+        if size > LIMIT {
             return Err("Review exceeds the 64 MiB limit".into());
         }
-        let document = std::str::from_utf8(&bytes).map_err(|error| error.to_string())?;
+        let verdict = match review.verdict {
+            Some(Verdict::LooksGood) => Some("looks_good"),
+            Some(Verdict::ChangesRequested) => Some("changes_requested"),
+            None => None,
+        };
         let transaction = self
             .connection
             .transaction_with_behavior(TransactionBehavior::Immediate)
             .map_err(database_error)?;
         let id = if let Some(id) = review.storage_id {
             let changed = transaction.execute(
-                "UPDATE reviews SET revision = ?1, document = ?2 WHERE id = ?3 AND root = ?4 AND revision = ?5",
-                params![revision, document, id, root, review.storage_revision],
+                "UPDATE reviews SET revision = ?1, summary = ?2, verdict = ?3 WHERE id = ?4 AND root = ?5 AND revision = ?6",
+                params![revision, review.summary, verdict, id, root, review.storage_revision],
             ).map_err(database_error)?;
             if changed != 1 {
                 return Err(
@@ -480,12 +544,46 @@ impl Store {
         } else {
             transaction
                 .execute(
-                    "INSERT INTO reviews (root, revision, document) VALUES (?1, ?2, ?3)",
-                    params![root, revision, document],
+                    "INSERT INTO reviews (root, revision, summary, verdict) VALUES (?1, ?2, ?3, ?4)",
+                    params![root, revision, review.summary, verdict],
                 )
                 .map_err(database_error)?;
             transaction.last_insert_rowid()
         };
+        // ponytail: replace comment rows per save; diff rows if snapshot writes become costly.
+        transaction
+            .execute("DELETE FROM comments WHERE review_id = ?1", [id])
+            .map_err(database_error)?;
+        {
+            let mut insert = transaction.prepare(
+                "INSERT INTO comments (review_id, id, position, body, resolved, anchor_revision, path, side,
+                    start_row, start_column, end_row, end_column, content)
+                 VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12, ?13)",
+            ).map_err(database_error)?;
+            for (position, comment) in review.comments.iter().enumerate() {
+                let anchor = &comment.anchor;
+                insert
+                    .execute(params![
+                        id,
+                        comment.id.to_string(),
+                        sql_index(position)?,
+                        comment.body,
+                        comment.resolved,
+                        anchor.revision,
+                        anchor.path.to_str().unwrap(),
+                        match anchor.side {
+                            Side::Before => "before",
+                            Side::After => "after",
+                        },
+                        sql_index(anchor.start.row)?,
+                        sql_index(anchor.start.column)?,
+                        sql_index(anchor.end.row)?,
+                        sql_index(anchor.end.column)?,
+                        anchor.content,
+                    ])
+                    .map_err(database_error)?;
+            }
+        }
         transaction.commit().map_err(database_error)?;
         review.storage_id = Some(id);
         review.storage_revision = revision;
@@ -497,26 +595,111 @@ fn database_error(error: rusqlite::Error) -> String {
     format!("Review database: {error}")
 }
 
-fn stored_review(row: &rusqlite::Row<'_>, root: &Path) -> Result<Review, String> {
+fn sql_index(value: usize) -> Result<i64, String> {
+    i64::try_from(value).map_err(|_| "Comment coordinate exceeds SQLite's integer range".into())
+}
+
+fn stored_index(row: &rusqlite::Row<'_>, index: usize) -> Result<usize, String> {
+    usize::try_from(row.get::<_, i64>(index).map_err(database_error)?)
+        .map_err(|_| "Stored comment coordinate exceeds this platform's range".into())
+}
+
+fn stored_review(
+    connection: &Connection,
+    row: &rusqlite::Row<'_>,
+    root: &Path,
+) -> Result<Review, String> {
     let id: i64 = row.get(0).map_err(database_error)?;
-    let stored_root: String = row.get(1).map_err(database_error)?;
+    let stored_root = row
+        .get_ref(1)
+        .map_err(database_error)?
+        .as_str()
+        .map_err(|error| error.to_string())?;
     let revision: i64 = row.get(2).map_err(database_error)?;
-    if id <= 0 || revision <= 0 || Path::new(&stored_root) != root {
+    if id <= 0 || revision <= 0 || Path::new(stored_root) != root {
         return Err("Invalid review database identity".into());
     }
-    let document = row.get_ref(3).map_err(database_error)?;
-    let bytes = document
+    let summary = row
+        .get_ref(3)
+        .map_err(database_error)?
         .as_str()
-        .map_err(|error| format!("Invalid stored review: {error}"))?
-        .as_bytes();
-    if bytes.len() > LIMIT {
+        .map_err(|error| error.to_string())?;
+    let comment_bytes: i64 = connection
+        .query_row(
+            "SELECT COALESCE(SUM(length(CAST(body AS BLOB)) + length(CAST(anchor_revision AS BLOB))
+            + length(CAST(path AS BLOB)) + length(CAST(content AS BLOB)) + 64), 0)
+         FROM comments WHERE review_id = ?1",
+            [id],
+            |row| row.get(0),
+        )
+        .map_err(database_error)?;
+    if comment_bytes < 0
+        || comment_bytes as u64 + stored_root.len() as u64 + summary.len() as u64 > LIMIT as u64
+    {
         return Err("Review exceeds the 64 MiB limit".into());
     }
-    let mut review = Review::from_json(bytes)?;
-    review.validate()?;
-    if review.root != root {
-        return Err("Stored review belongs to a different repository".into());
+    let mut review = Review {
+        root: root.to_owned(),
+        ..Review::default()
+    };
+    review.summary = summary.to_owned();
+    review.verdict = match row
+        .get::<_, Option<String>>(4)
+        .map_err(database_error)?
+        .as_deref()
+    {
+        Some("looks_good") => Some(Verdict::LooksGood),
+        Some("changes_requested") => Some(Verdict::ChangesRequested),
+        None => None,
+        _ => return Err("Invalid stored review verdict".into()),
+    };
+    let mut statement = connection
+        .prepare(
+            "SELECT id, position, body, resolved, anchor_revision, path, side,
+            start_row, start_column, end_row, end_column, content
+         FROM comments WHERE review_id = ?1 ORDER BY position",
+        )
+        .map_err(database_error)?;
+    let mut rows = statement.query([id]).map_err(database_error)?;
+    while let Some(row) = rows.next().map_err(database_error)? {
+        if stored_index(row, 1)? != review.comments.len() {
+            return Err("Invalid stored comment order".into());
+        }
+        let side = match row.get::<_, String>(6).map_err(database_error)?.as_str() {
+            "before" => Side::Before,
+            "after" => Side::After,
+            _ => return Err("Invalid stored comment side".into()),
+        };
+        let resolved = match row.get::<_, i64>(3).map_err(database_error)? {
+            0 => false,
+            1 => true,
+            _ => return Err("Invalid stored comment resolution".into()),
+        };
+        review.comments.push(Comment {
+            id: row
+                .get::<_, String>(0)
+                .map_err(database_error)?
+                .parse::<u64>()
+                .map_err(|_| "Invalid stored comment ID")?,
+            body: row.get(2).map_err(database_error)?,
+            resolved,
+            anchor: Anchor {
+                revision: row.get(4).map_err(database_error)?,
+                path: PathBuf::from(row.get::<_, String>(5).map_err(database_error)?),
+                side,
+                start: crate::plugin::TextPosition {
+                    row: stored_index(row, 7)?,
+                    column: stored_index(row, 8)?,
+                },
+                end: crate::plugin::TextPosition {
+                    row: stored_index(row, 9)?,
+                    column: stored_index(row, 10)?,
+                },
+                content: row.get(11).map_err(database_error)?,
+            },
+        });
     }
+    review.validate()?;
     review.storage_id = Some(id);
     review.storage_revision = revision;
     Ok(review)

tests/unit/review.rs

@@ -151,6 +151,10 @@ fn incompatible_database_schemas_are_preserved() {
     for schema in [
         "CREATE TABLE other (value TEXT); PRAGMA user_version = 42",
         "CREATE TABLE other (value TEXT); PRAGMA user_version = 1",
+        "CREATE TABLE reviews (id INTEGER PRIMARY KEY, root TEXT NOT NULL,
+            revision INTEGER NOT NULL, document TEXT NOT NULL) STRICT;
+         INSERT INTO reviews VALUES (1, '/old/repo', 1, '{\"version\":3}');
+         PRAGMA user_version = 1",
         "",
     ] {
         let directory = directory();
@@ -165,44 +169,73 @@ fn incompatible_database_schemas_are_preserved() {
 }
 
 #[test]
-fn malformed_or_mismatched_database_documents_are_rejected() {
-    let other = directory();
+fn malformed_database_rows_are_rejected_before_loading_snapshots() {
     let directory = directory();
     let root = directory.0.canonicalize().unwrap();
     let mut store = Store::open(&root.join("storage")).unwrap();
     let mut review = Review::new(root.clone());
+    review.comments.push(Comment {
+        id: 1,
+        body: "Feedback".into(),
+        resolved: false,
+        anchor: Anchor {
+            revision: "snapshot".into(),
+            path: "file.rs".into(),
+            side: Side::After,
+            start: crate::plugin::TextPosition { row: 0, column: 0 },
+            end: crate::plugin::TextPosition { row: 0, column: 1 },
+            content: "界\n".into(),
+        },
+    });
     store.save(&mut review).unwrap();
     let id = review.storage_id.unwrap();
-    let mut wrong_root = serde_json::to_value(&review).unwrap();
-    wrong_root["root"] = serde_json::json!(other.0.canonicalize().unwrap());
-    let mut invalid_version = serde_json::to_value(&review).unwrap();
-    invalid_version["version"] = 1.into();
-    let mut inherited_identity = serde_json::to_value(&review).unwrap();
-    inherited_identity["storage_id"] = id.into();
-    for document in [
-        "{".to_owned(),
-        serde_json::to_string(&wrong_root).unwrap(),
-        serde_json::to_string(&invalid_version).unwrap(),
-        serde_json::to_string(&inherited_identity).unwrap(),
+    for update in [
+        "resolved = 2",
+        "side = 'invalid'",
+        "id = '0'",
+        "id = '18446744073709551616'",
+        "start_row = -1",
+        "end_column = -1",
     ] {
-        store
-            .connection
-            .execute(
-                "UPDATE reviews SET document = ?1 WHERE id = ?2",
-                params![document, id],
-            )
-            .unwrap();
-        assert!(store.load(&root, id).is_err());
-        assert!(store.list(&root).is_err());
+        assert!(
+            store
+                .connection
+                .execute(
+                    &format!("UPDATE comments SET {update} WHERE review_id = ?1"),
+                    [id],
+                )
+                .is_err()
+        );
+        assert_eq!(store.load(&root, id).unwrap().comments, review.comments);
     }
     store
         .connection
         .execute_batch("PRAGMA ignore_check_constraints = ON")
         .unwrap();
+    for update in [
+        "path = '../outside'",
+        "end_column = 2",
+        "resolved = 2",
+        "side = 'invalid'",
+        "anchor_revision = ''",
+        "id = '0'",
+        "position = 1",
+    ] {
+        store
+            .connection
+            .execute(
+                &format!("UPDATE comments SET {update} WHERE review_id = ?1"),
+                [id],
+            )
+            .unwrap();
+        assert!(store.load(&root, id).is_err(), "{update}");
+        assert!(store.list(&root).is_err(), "{update}");
+        store.save(&mut review).unwrap();
+    }
     store
         .connection
         .execute(
-            "UPDATE reviews SET document = CAST(zeroblob(?1) AS TEXT) WHERE id = ?2",
+            "UPDATE comments SET content = CAST(zeroblob(?1) AS TEXT) WHERE review_id = ?2",
             params![LIMIT as i64 + 1, id],
         )
         .unwrap();
@@ -406,43 +439,82 @@ fn persistence_validates_anchors_and_preserves_files_on_failure() {
 }
 
 #[test]
-fn stored_version_two_is_rejected_without_rewriting_it() {
+fn comment_rows_preserve_order_full_ids_and_atomic_replacement() {
     let directory = directory();
     let root = directory.0.canonicalize().unwrap();
     let mut store = Store::open(&root.join("storage")).unwrap();
     let mut review = Review::new(root.clone());
+    review.comments = [u64::MAX, 7]
+        .into_iter()
+        .map(|id| Comment {
+            id,
+            body: format!("Feedback {id}"),
+            resolved: id == 7,
+            anchor: Anchor {
+                revision: "snapshot".into(),
+                path: "source.rs".into(),
+                side: Side::Before,
+                start: crate::plugin::TextPosition { row: 0, column: 0 },
+                end: crate::plugin::TextPosition { row: 0, column: 1 },
+                content: "界\n".into(),
+            },
+        })
+        .collect();
+    review.summary = "Original".into();
     store.save(&mut review).unwrap();
-    let mut old = serde_json::to_value(&review).unwrap();
-    old["version"] = 2.into();
-    let document = serde_json::to_string(&old).unwrap();
+    let id = review.storage_id.unwrap();
+    assert_eq!(store.load(&root, id).unwrap().comments, review.comments);
+    assert!(
+        store
+            .connection
+            .execute(
+                "UPDATE comments SET review_id = ?1 WHERE review_id = ?2",
+                params![id + 1, id],
+            )
+            .is_err()
+    );
+    assert!(
+        store
+            .connection
+            .execute("UPDATE comments SET id = '7' WHERE review_id = ?1", [id],)
+            .is_err()
+    );
     store
         .connection
-        .execute(
-            "UPDATE reviews SET document = ?1 WHERE id = ?2",
-            params![document, review.storage_id.unwrap()],
+        .execute_batch(
+            "CREATE TEMP TRIGGER reject_comment BEFORE INSERT ON comments
+         WHEN NEW.id = '7' BEGIN SELECT RAISE(ABORT, 'blocked comment'); END",
         )
         .unwrap();
+    let revision = review.storage_revision;
+    let original = review.comments.clone();
+    review.summary = "Unsaved".into();
+    review.comments[0].body = "Changed".into();
     assert!(
         store
-            .load(&root, review.storage_id.unwrap())
+            .save(&mut review)
             .unwrap_err()
-            .contains("Unsupported review version 2")
+            .contains("blocked comment")
     );
-    assert!(
-        store
-            .list(&root)
-            .unwrap_err()
-            .contains("Unsupported review version 2")
-    );
-    let preserved: String = store
+    assert_eq!(review.storage_revision, revision);
+    let loaded = store.load(&root, id).unwrap();
+    assert_eq!(loaded.summary, "Original");
+    assert_eq!(loaded.comments, original);
+    assert_eq!(loaded.storage_revision, revision);
+    store
         .connection
-        .query_row(
-            "SELECT document FROM reviews WHERE id = ?1",
-            [review.storage_id.unwrap()],
-            |row| row.get(0),
-        )
+        .execute_batch("DROP TRIGGER reject_comment")
         .unwrap();
-    assert_eq!(preserved, document);
+    review.comments.reverse();
+    store.save(&mut review).unwrap();
+    assert_eq!(store.load(&root, id).unwrap().comments, review.comments);
+    review.comments.pop();
+    store.save(&mut review).unwrap();
+    assert_eq!(store.load(&root, id).unwrap().comments, review.comments);
+    review.comments.clear();
+    store.save(&mut review).unwrap();
+    assert!(store.load(&root, id).unwrap().comments.is_empty());
+    assert_eq!(store.list(&root).unwrap()[0].summary, "Unsaved");
 }
 
 #[test]