From e2ebd3331ec094b9af2fdc907836b14af3127917 Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Wed, 2 Sep 2026 12:27:39 -0400 Subject: [PATCH 1/2] feat(annotations): add anchored-annotation store and resolver --- Cargo.lock | 10 + Cargo.toml | 8 +- git-workon-annotations/Cargo.toml | 37 ++ git-workon-annotations/src/anchor.rs | 253 ++++++++++ git-workon-annotations/src/error.rs | 49 ++ git-workon-annotations/src/lib.rs | 185 +++++++ git-workon-annotations/src/schema.rs | 113 +++++ git-workon-annotations/src/store.rs | 704 +++++++++++++++++++++++++++ 8 files changed, 1358 insertions(+), 1 deletion(-) create mode 100644 git-workon-annotations/Cargo.toml create mode 100644 git-workon-annotations/src/anchor.rs create mode 100644 git-workon-annotations/src/error.rs create mode 100644 git-workon-annotations/src/lib.rs create mode 100644 git-workon-annotations/src/schema.rs create mode 100644 git-workon-annotations/src/store.rs diff --git a/Cargo.lock b/Cargo.lock index 53d0fd00..738c33f9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -930,6 +930,16 @@ dependencies = [ "unicode-width 0.2.2", ] +[[package]] +name = "git-workon-annotations" +version = "0.1.0" +dependencies = [ + "miette", + "rusqlite", + "tempfile", + "thiserror 2.0.19", +] + [[package]] name = "git-workon-fixture" version = "0.1.0" diff --git a/Cargo.toml b/Cargo.toml index 92a63124..d0d94815 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,7 +1,13 @@ [workspace] resolver = "2" default-members = ["git-workon"] -members = ["git-workon", "git-workon-lib", "git-workon-fixture", "git-workon-review"] +members = [ + "git-workon", + "git-workon-lib", + "git-workon-fixture", + "git-workon-review", + "git-workon-annotations", +] [workspace.package] authors = ["Eric Eldredge "] diff --git a/git-workon-annotations/Cargo.toml b/git-workon-annotations/Cargo.toml new file mode 100644 index 00000000..dbe26701 --- /dev/null +++ b/git-workon-annotations/Cargo.toml @@ -0,0 +1,37 @@ +[package] +authors.workspace = true +categories = ["command-line-utilities", "development-tools"] +description = "Anchored-annotation store shared by review comments and walkthroughs" +edition.workspace = true +homepage.workspace = true +keywords = ["git", "review", "annotations", "workon"] +license.workspace = true +name = "git-workon-annotations" +repository.workspace = true +rust-version.workspace = true +version = "0.1.0" +include = [ + "src/**/*", + "Cargo.toml", + "LICENSE*", +] +# Not yet published: this crate has no bin target yet (the MCP server lands in a later +# slice) and the store schema is still settling. Follow the ADR-033 posture: flipping to +# publish is a deferred sub-decision, not an oversight to fix later. +publish = false + +[lib] +name = "workon_annotations" + +[dependencies] +miette.workspace = true +rusqlite.workspace = true +thiserror.workspace = true + +[package.metadata.dist] +# Redundant with publish = false today; load-bearing once a bin target and the publish flip +# land (see ADR-033's posture, adopted here) so cargo-dist doesn't silently start shipping it. +dist = false + +[dev-dependencies] +tempfile = "3" diff --git a/git-workon-annotations/src/anchor.rs b/git-workon-annotations/src/anchor.rs new file mode 100644 index 00000000..66138fdb --- /dev/null +++ b/git-workon-annotations/src/anchor.rs @@ -0,0 +1,253 @@ +//! Pure resolver: given a stored [`Anchor`] and the current lines of the side it targets, +//! find where the target line lives now. No I/O, no store access — this is the unit-test +//! center of the crate (ADR-039's anchoring decision). +//! +//! Resolution order: exact match at the stored line, then a windowed outward scan for the +//! target text scored by surrounding context, then a repeat of that scan with whitespace +//! trimmed from both target and context, else [`Anchoring::Orphaned`]. + +use crate::{Anchor, Anchoring}; + +/// Where `anchor`'s target line resolved to against `lines` (1-based, matching +/// [`Anchor::lineno`]), and how confidently. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct Resolution { + /// `None` only when `anchoring` is [`Anchoring::Orphaned`]. + pub lineno: Option, + pub anchoring: Anchoring, +} + +/// Resolve `anchor` against `lines` — the current content of the side (old or new) and file +/// [`Anchor::new_side`]/[`Anchor::path`] name, one entry per line, no trailing newlines. +pub fn resolve(anchor: &Anchor, lines: &[&str]) -> Resolution { + let original_idx = anchor.lineno.saturating_sub(1) as usize; + + if lines.get(original_idx) == Some(&anchor.target.as_str()) + && context_matches(anchor, lines, original_idx, false) + { + return Resolution { + lineno: Some(original_idx as u32 + 1), + anchoring: Anchoring::Exact, + }; + } + + if let Some(idx) = scan(anchor, lines, original_idx, false) { + return Resolution { + lineno: Some(idx as u32 + 1), + anchoring: Anchoring::Shifted { + from: anchor.lineno, + }, + }; + } + + if let Some(idx) = scan(anchor, lines, original_idx, true) { + return Resolution { + lineno: Some(idx as u32 + 1), + anchoring: Anchoring::Shifted { + from: anchor.lineno, + }, + }; + } + + Resolution { + lineno: None, + anchoring: Anchoring::Orphaned, + } +} + +/// Outward scan from `origin`, nearest index first, for lines equal to the target (trimmed if +/// `whitespace_tolerant`). Among matches, picks the highest-scoring by context; ties broken by +/// distance from `origin` (the scan order already visits nearest first, so the first +/// max-score match wins). A match needs `score >= 1` unless the target text is unique among +/// candidates in `lines` (only one occurrence to choose from — nothing to disambiguate). +fn scan( + anchor: &Anchor, + lines: &[&str], + origin: usize, + whitespace_tolerant: bool, +) -> Option { + let target_eq = |line: &str| -> bool { + if whitespace_tolerant { + line.trim() == anchor.target.trim() + } else { + line == anchor.target + } + }; + + let candidates: Vec = distance_order(origin, lines.len()) + .into_iter() + .filter(|&idx| target_eq(lines[idx])) + .collect(); + + if candidates.is_empty() { + return None; + } + if candidates.len() == 1 { + return Some(candidates[0]); + } + + let mut best: Option<(usize, u32)> = None; + for idx in candidates { + let score = context_score(anchor, lines, idx, whitespace_tolerant); + if score >= 1 + && best + .map(|(_, best_score)| score > best_score) + .unwrap_or(true) + { + best = Some((idx, score)); + } + } + best.map(|(idx, _)| idx) +} + +/// Indices `0..len`, ordered by ascending distance from `origin` (origin first, then +/// alternating -1/+1 out from it). +fn distance_order(origin: usize, len: usize) -> Vec { + let mut order = Vec::with_capacity(len); + if len == 0 { + return order; + } + let origin = origin.min(len - 1); + order.push(origin); + let mut back = origin; + let mut forward = origin; + loop { + let mut moved = false; + if back > 0 { + back -= 1; + order.push(back); + moved = true; + } + if forward + 1 < len { + forward += 1; + order.push(forward); + moved = true; + } + if !moved { + break; + } + } + order +} + +fn context_matches(anchor: &Anchor, lines: &[&str], idx: usize, whitespace_tolerant: bool) -> bool { + let want = anchor.before.len() as u32 + anchor.after.len() as u32; + context_score(anchor, lines, idx, whitespace_tolerant) == want +} + +/// Count of `anchor.before`/`anchor.after` entries that match the corresponding line around +/// candidate `idx` (missing lines at file edges just don't score). +fn context_score(anchor: &Anchor, lines: &[&str], idx: usize, whitespace_tolerant: bool) -> u32 { + let eq = |a: &str, b: &str| { + if whitespace_tolerant { + a.trim() == b.trim() + } else { + a == b + } + }; + + let mut score = 0; + for (offset, expected) in anchor.before.iter().rev().enumerate() { + let pos = idx.checked_sub(offset + 1); + if let Some(pos) = pos { + if lines.get(pos).is_some_and(|line| eq(line, expected)) { + score += 1; + } + } + } + for (offset, expected) in anchor.after.iter().enumerate() { + let pos = idx + offset + 1; + if lines.get(pos).is_some_and(|line| eq(line, expected)) { + score += 1; + } + } + score +} + +#[cfg(test)] +mod tests { + use super::*; + + fn anchor(lineno: u32, target: &str, before: &[&str], after: &[&str]) -> Anchor { + Anchor { + path: "f.rs".into(), + new_side: true, + lineno, + end_lineno: lineno, + target: target.into(), + before: before.iter().map(|s| s.to_string()).collect(), + after: after.iter().map(|s| s.to_string()).collect(), + } + } + + #[test] + fn exact_match_at_stored_line() { + let a = anchor(2, "target", &["before"], &["after"]); + let lines = vec!["before", "target", "after"]; + let r = resolve(&a, &lines); + assert_eq!(r.lineno, Some(2)); + assert_eq!(r.anchoring, Anchoring::Exact); + } + + #[test] + fn shifted_downward() { + // Two lines inserted above the target: target used to be line 2, now line 4. + let a = anchor(2, "target", &["before"], &["after"]); + let lines = vec!["inserted-1", "inserted-2", "before", "target", "after"]; + let r = resolve(&a, &lines); + assert_eq!(r.lineno, Some(4)); + assert_eq!(r.anchoring, Anchoring::Shifted { from: 2 }); + } + + #[test] + fn shifted_upward() { + // A line removed above the target: target used to be line 3, now line 2. + let a = anchor(3, "target", &["before"], &["after"]); + let lines = vec!["before", "target", "after"]; + let r = resolve(&a, &lines); + assert_eq!(r.lineno, Some(2)); + assert_eq!(r.anchoring, Anchoring::Shifted { from: 3 }); + } + + #[test] + fn whitespace_only_change_resolves_shifted() { + let a = anchor(1, " target", &[], &["after"]); + let lines = vec!["target", "after"]; + let r = resolve(&a, &lines); + assert_eq!(r.lineno, Some(1)); + assert_eq!(r.anchoring, Anchoring::Shifted { from: 1 }); + } + + #[test] + fn duplicate_target_disambiguated_by_context() { + // "dup" appears at idx 1 and idx 3; the stored line (idx 4, 1-based 5) holds neither, + // so this exercises the scored scan directly. Only idx 3 has the right before-context + // ("ctx-a" at idx 2), so it wins over the unscored duplicate at idx 1. + let a = anchor(5, "dup", &["ctx-a"], &[]); + let lines = vec!["other", "dup", "ctx-a", "dup", "unrelated"]; + let r = resolve(&a, &lines); + assert_eq!(r.lineno, Some(4)); + assert!(matches!(r.anchoring, Anchoring::Shifted { .. })); + } + + #[test] + fn ambiguous_duplicate_with_no_context_signal_picks_nearest() { + // Neither "dup" occurrence has any matching context, so score can't disambiguate; + // the scan still resolves (score requirement is waived only when unique — here it + // isn't — so this exercises "no candidate clears score >= 1" -> Orphaned). + let a = anchor(10, "dup", &["never-matches"], &[]); + let lines = vec!["dup", "x", "dup"]; + let r = resolve(&a, &lines); + assert_eq!(r.lineno, None); + assert_eq!(r.anchoring, Anchoring::Orphaned); + } + + #[test] + fn orphan_when_target_absent() { + let a = anchor(1, "gone", &[], &[]); + let lines = vec!["still-here"]; + let r = resolve(&a, &lines); + assert_eq!(r.lineno, None); + assert_eq!(r.anchoring, Anchoring::Orphaned); + } +} diff --git a/git-workon-annotations/src/error.rs b/git-workon-annotations/src/error.rs new file mode 100644 index 00000000..dcfbd2e0 --- /dev/null +++ b/git-workon-annotations/src/error.rs @@ -0,0 +1,49 @@ +use miette::Diagnostic; +use thiserror::Error; + +/// Result type alias using [`AnnotationsError`]. +pub type Result = std::result::Result; + +/// Errors from the annotation store: opening the database, migrating its schema, and +/// running the CRUD/resolver operations on top of it. Follows the two-layer pattern +/// (ADR-008): a concrete enum, no `.into_diagnostic()` calls in this crate. +#[derive(Error, Diagnostic, Debug)] +pub enum AnnotationsError { + /// Creating the database's parent directory failed. + #[error("failed to create annotations database directory at '{path}'")] + #[diagnostic(code(workon::annotations::db_dir_failed))] + DbDirFailed { + path: String, + #[source] + source: std::io::Error, + }, + + /// Opening the sqlite connection failed. + #[error("failed to open annotations database at '{path}'")] + #[diagnostic(code(workon::annotations::open_failed))] + OpenFailed { + path: String, + #[source] + source: rusqlite::Error, + }, + + /// Applying the schema (or a migration step) failed. + #[error("failed to migrate annotations database schema")] + #[diagnostic(code(workon::annotations::migration_failed))] + MigrationFailed(#[source] rusqlite::Error), + + /// A CRUD or query statement against the store failed. + #[error("annotation store query failed")] + #[diagnostic(code(workon::annotations::query_failed))] + QueryFailed(#[source] rusqlite::Error), + + /// A write transaction failed to commit (or begin/rollback). + #[error("annotation store write transaction failed")] + #[diagnostic(code(workon::annotations::transaction_failed))] + TransactionFailed(#[source] rusqlite::Error), + + /// A `uid` (annotation, or parent for a reply) named by the caller doesn't exist. + #[error("no annotation with uid '{uid}'")] + #[diagnostic(code(workon::annotations::not_found))] + NotFound { uid: String }, +} diff --git a/git-workon-annotations/src/lib.rs b/git-workon-annotations/src/lib.rs new file mode 100644 index 00000000..3d27e031 --- /dev/null +++ b/git-workon-annotations/src/lib.rs @@ -0,0 +1,185 @@ +//! Anchored-annotation store shared by review comments and walkthrough content +//! (`git-workon-review`'s comment threads and the explain-diff-style tour/chapter prose). +//! +//! One substrate, two uses (ADR-039): a comment is `AnnotationKind::Comment`; a walkthrough +//! stop is `AnnotationKind::TourStop` ordered by `(tour, seq)`; a chapter is per-changeset +//! prose, `AnnotationKind::Chapter`. All three share one table, one anchoring scheme, and one +//! store API. +//! +//! This crate is serde-free and git2-free: [`store::AnnotationStore`] takes a `commondir` +//! path (the caller resolves it, e.g. via `git2::Repository::commondir()`), and the types +//! here are plain structs with rusqlite row mapping. JSON only enters at the MCP boundary +//! (a later slice, hosted as a second bin target in this crate — see ADR-039). +//! +//! ## Status +//! +//! Slice 1 (this crate's scaffold): types, schema, [`store::AnnotationStore`], and the pure +//! resolver in [`anchor`]. Nothing here is wired into `git-workon-review` yet. + +pub mod anchor; +pub mod error; +mod schema; +pub mod store; + +pub use error::{AnnotationsError, Result}; + +/// Identity of the changeset an annotation is anchored to: branch name plus whether it names +/// the uncommitted layer. Mirrors `git-workon-review`'s `app::ChangesetIdentity` (name alone +/// is ambiguous — the uncommitted layer is named after the current branch, so it collides +/// with that branch's committed changeset without this flag). +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct ChangesetKey { + name: String, + uncommitted: bool, +} + +impl ChangesetKey { + /// Build a key naming `name`'s committed changeset (`uncommitted: false`) or its + /// uncommitted layer (`uncommitted: true`). + pub fn new(name: impl Into, uncommitted: bool) -> Self { + Self { + name: name.into(), + uncommitted, + } + } + + pub fn name(&self) -> &str { + &self.name + } + + pub fn uncommitted(&self) -> bool { + self.uncommitted + } +} + +/// What an annotation is for. A walkthrough tour is annotations sharing a `tour` name, +/// ordered by `seq`; a chapter is per-changeset prose. See ADR-039. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub enum AnnotationKind { + Comment, + TourStop, + Chapter, +} + +impl AnnotationKind { + fn as_str(self) -> &'static str { + match self { + AnnotationKind::Comment => "comment", + AnnotationKind::TourStop => "tour_stop", + AnnotationKind::Chapter => "chapter", + } + } + + fn from_str(s: &str) -> Option { + match s { + "comment" => Some(AnnotationKind::Comment), + "tour_stop" => Some(AnnotationKind::TourStop), + "chapter" => Some(AnnotationKind::Chapter), + _ => None, + } + } +} + +/// Persisted lifecycle state. `Orphaned` (an anchor that no longer resolves) is deliberately +/// NOT here — it's derived per load by [`anchor::resolve`], not persisted, so a discarded +/// edit that restores the original content un-orphans the annotation for free. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub enum Status { + Open, + Resolved, +} + +impl Status { + fn as_str(self) -> &'static str { + match self { + Status::Open => "open", + Status::Resolved => "resolved", + } + } + + fn from_str(s: &str) -> Option { + match s { + "open" => Some(Status::Open), + "resolved" => Some(Status::Resolved), + _ => None, + } + } +} + +/// How an anchor resolved against the current file content this load. Derived by +/// [`anchor::resolve`]; never persisted (only the anchor's captured target/context is). +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub enum Anchoring { + /// The stored line number still holds the stored target text and context. + Exact, + /// The target text was found elsewhere; `from` is the line number the anchor was + /// captured at, for a "moved from line N" UI hint. + Shifted { from: u32 }, + /// No occurrence of the target text (exact or whitespace-tolerant) resolved with enough + /// context confidence. Renders as "unanchored", never silently wrong. + Orphaned, +} + +/// A captured location: the target line's text plus up to 3 lines of context each way, over +/// one side (old or new) of one file. `before`/`after` are top-to-bottom reading order +/// (`before[0]` is furthest from the target, `after.last()` is furthest); each holds 0-3 +/// lines, fewer at file edges. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Anchor { + pub path: String, + /// Which side of the diff the anchor targets — the new-file side (`true`) or the + /// old-file side (`false`). Resolution is per (file, role) view: see the gotcha in + /// ADR-039 about `FileView::load`'s role-dependent "new side". + pub new_side: bool, + pub lineno: u32, + /// End of the anchored span for a multi-line selection; equal to `lineno` for a + /// single-line anchor. The resolver only tracks `target`/context for `lineno` — callers + /// recompute the span length against the resolved line. + pub end_lineno: u32, + pub target: String, + pub before: Vec, + pub after: Vec, +} + +/// One annotation: a comment, tour stop, or chapter. `anchor` is `None` for a chapter (prose +/// scoped to the whole changeset, not a line) and for a reply (a reply inherits its parent's +/// anchor implicitly; storing it again would just drift out of sync on re-resolve). +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Annotation { + pub uid: String, + pub kind: AnnotationKind, + pub status: Status, + pub parent_uid: Option, + pub changeset: ChangesetKey, + pub anchor: Option, + pub body: String, + pub author: String, + pub tour: Option, + pub seq: Option, + pub created_at: i64, + pub updated_at: i64, +} + +/// Fields for a new top-level annotation ([`store::AnnotationStore::insert`]). `uid`, +/// `status` (always starts `Open`), `created_at`, and `updated_at` are assigned by the +/// store. Use [`store::AnnotationStore::reply`] for replies, which take a `parent_uid` +/// instead of an anchor. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct NewAnnotation { + pub kind: AnnotationKind, + pub changeset: ChangesetKey, + pub anchor: Option, + pub body: String, + pub author: String, + pub tour: Option, + pub seq: Option, +} + +/// The store's write-visibility fingerprint (`store::AnnotationStore::fingerprint`). The TUI +/// watcher polls this to decide whether to reload; it changes only when some OTHER +/// connection committed a write (see the `PRAGMA data_version` note on `fingerprint`). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct Fingerprint { + pub data_version: i64, + pub revision: i64, +} diff --git a/git-workon-annotations/src/schema.rs b/git-workon-annotations/src/schema.rs new file mode 100644 index 00000000..f970802d --- /dev/null +++ b/git-workon-annotations/src/schema.rs @@ -0,0 +1,113 @@ +//! Schema DDL and the fixed-content hash used to detect an anchor's captured context. DDL +//! idioms (batch-execute a `CREATE TABLE`/`CREATE INDEX` script, `busy_timeout` + +//! `journal_mode=WAL` for the writer, `READ_ONLY | NO_MUTEX` for a reader) follow +//! `git-workon-fixture`'s sqlite graphite-metadata writer and `git-workon-lib`'s reader +//! (`stack/graphite.rs`). + +use rusqlite::Connection; + +use crate::error::{AnnotationsError, Result}; + +/// Bumped when the DDL below changes in a way existing databases need migrating for. Slice 1 +/// ships version 1; there is no migration path yet because there is nothing to migrate from. +pub const SCHEMA_VERSION: i64 = 1; + +const DDL: &str = r#" +CREATE TABLE IF NOT EXISTS annotation ( + uid TEXT PRIMARY KEY, + kind TEXT NOT NULL, + status TEXT NOT NULL, + parent_uid TEXT REFERENCES annotation(uid), + changeset_name TEXT NOT NULL, + changeset_uncommitted INTEGER NOT NULL, + anchor_path TEXT, + anchor_new_side INTEGER, + anchor_lineno INTEGER, + anchor_end_lineno INTEGER, + anchor_target TEXT, + anchor_before TEXT, + anchor_after TEXT, + anchor_ctx_hash TEXT, + body TEXT NOT NULL, + author TEXT NOT NULL, + tour TEXT, + seq INTEGER, + created_at INTEGER NOT NULL, + updated_at INTEGER NOT NULL +); + +CREATE INDEX IF NOT EXISTS idx_annotation_changeset_path + ON annotation (changeset_name, changeset_uncommitted, anchor_path); + +CREATE INDEX IF NOT EXISTS idx_annotation_parent + ON annotation (parent_uid); + +CREATE INDEX IF NOT EXISTS idx_annotation_tour + ON annotation (tour, seq); + +CREATE TABLE IF NOT EXISTS meta ( + id INTEGER PRIMARY KEY CHECK (id = 1), + schema_version INTEGER NOT NULL, + revision INTEGER NOT NULL +); +"#; + +/// Apply the DDL (idempotent — every statement is `IF NOT EXISTS`) and seed the singleton +/// `meta` row if this is a fresh database. +pub fn migrate(conn: &Connection) -> Result<()> { + conn.execute_batch(DDL) + .map_err(AnnotationsError::MigrationFailed)?; + conn.execute( + "INSERT OR IGNORE INTO meta (id, schema_version, revision) VALUES (1, ?1, 0)", + [SCHEMA_VERSION], + ) + .map_err(AnnotationsError::MigrationFailed)?; + Ok(()) +} + +/// FNV-1a 64-bit over `before` + `target` + `after` (joined with `\n`), stored alongside the +/// captured context as `ctx_hash`. Deliberately NOT `std::hash::DefaultHasher`: that hasher's +/// output isn't guaranteed stable across Rust releases, and this hash is persisted to disk and +/// compared against on a later run, possibly built by a different toolchain. +pub fn ctx_hash(before: &[String], target: &str, after: &[String]) -> String { + const FNV_OFFSET: u64 = 0xcbf2_9ce4_8422_2325; + const FNV_PRIME: u64 = 0x0000_0100_0000_01b3; + + let mut hash = FNV_OFFSET; + let mut feed = |s: &str| { + for byte in s.as_bytes() { + hash ^= u64::from(*byte); + hash = hash.wrapping_mul(FNV_PRIME); + } + // A separator byte outside ASCII text keeps "a","bc" from hashing the same as "ab","c". + hash ^= 0xff; + hash = hash.wrapping_mul(FNV_PRIME); + }; + for line in before { + feed(line); + } + feed(target); + for line in after { + feed(line); + } + format!("{hash:016x}") +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn ctx_hash_is_deterministic() { + let a = ctx_hash(&["x".into(), "y".into()], "target", &["z".into()]); + let b = ctx_hash(&["x".into(), "y".into()], "target", &["z".into()]); + assert_eq!(a, b); + } + + #[test] + fn ctx_hash_distinguishes_boundary_shifts() { + let a = ctx_hash(&["ab".into()], "c", &[]); + let b = ctx_hash(&["a".into()], "bc", &[]); + assert_ne!(a, b); + } +} diff --git a/git-workon-annotations/src/store.rs b/git-workon-annotations/src/store.rs new file mode 100644 index 00000000..8eb895d0 --- /dev/null +++ b/git-workon-annotations/src/store.rs @@ -0,0 +1,704 @@ +//! [`AnnotationStore`]: the sqlite-backed CRUD/query API over the `annotation` table (ADR-039). +//! +//! The database lives at `/workon-review/annotations.db` — `commondir`, not +//! `repo.path()`, so every worktree of a repo shares one store (the same discipline +//! `git-workon-lib`'s graphite reader uses for `.graphite_metadata.db`). This crate doesn't +//! resolve `commondir` itself (no git2 dep); callers pass it in, typically from +//! `git2::Repository::commondir()`. + +use std::path::{Path, PathBuf}; +use std::time::{SystemTime, UNIX_EPOCH}; + +use rusqlite::{params, Connection, OpenFlags, OptionalExtension, Row}; + +use crate::error::{AnnotationsError, Result}; +use crate::{Anchor, Annotation, AnnotationKind, ChangesetKey, Fingerprint, NewAnnotation, Status}; + +/// One tour stop or chapter to write as part of [`AnnotationStore::put_walkthrough`]. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct TourStop { + pub anchor: Anchor, + pub body: String, + pub author: String, + pub seq: i64, +} + +/// A full walkthrough write: an optional per-changeset chapter plus its ordered tour stops, +/// applied in one transaction (a partial write — chapter with no stops, or stops without a +/// chapter — would leave the TUI's watcher observing a half-authored tour). +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Walkthrough { + pub changeset: ChangesetKey, + pub tour: String, + pub chapter: Option, + pub chapter_author: Option, + pub stops: Vec, +} + +pub struct AnnotationStore { + conn: Connection, +} + +impl AnnotationStore { + /// Open (creating if absent) the store at `/workon-review/annotations.db` for + /// reading and writing: creates the parent directory, migrates the schema, and sets + /// `journal_mode=WAL` + `busy_timeout=3000`ms so concurrent writers (the TUI, an MCP + /// server) block briefly instead of erroring. + pub fn open(commondir: &Path) -> Result { + let path = db_path(commondir); + if let Some(dir) = path.parent() { + std::fs::create_dir_all(dir).map_err(|source| AnnotationsError::DbDirFailed { + path: dir.display().to_string(), + source, + })?; + } + let conn = Connection::open(&path).map_err(|source| AnnotationsError::OpenFailed { + path: path.display().to_string(), + source, + })?; + conn.busy_timeout(std::time::Duration::from_millis(3000)) + .map_err(AnnotationsError::MigrationFailed)?; + conn.pragma_update(None, "journal_mode", "WAL") + .map_err(AnnotationsError::MigrationFailed)?; + // The bundled sqlite build happens to default foreign_keys on; don't depend on a + // compile-time flag for the parent_uid constraint the delete walk relies on. + conn.pragma_update(None, "foreign_keys", true) + .map_err(AnnotationsError::MigrationFailed)?; + crate::schema::migrate(&conn)?; + Ok(Self { conn }) + } + + /// Open the store read-only (`SQLITE_OPEN_READ_ONLY | SQLITE_OPEN_NO_MUTEX`, the flags + /// `git-workon-lib`'s graphite-metadata reader uses). Errors if the database doesn't + /// exist yet — there's nothing to read, and read-only can't create it. + pub fn open_read_only(commondir: &Path) -> Result { + let path = db_path(commondir); + let conn = Connection::open_with_flags( + &path, + OpenFlags::SQLITE_OPEN_READ_ONLY | OpenFlags::SQLITE_OPEN_NO_MUTEX, + ) + .map_err(|source| AnnotationsError::OpenFailed { + path: path.display().to_string(), + source, + })?; + Ok(Self { conn }) + } + + /// Insert a new top-level annotation (comment, tour stop, or chapter). Starts `Open`. + pub fn insert(&self, new: NewAnnotation) -> Result { + let now = now(); + let fields = anchor_fields(new.anchor.as_ref()); + let uid: String = self + .conn + .query_row( + "INSERT INTO annotation ( + uid, kind, status, parent_uid, changeset_name, changeset_uncommitted, + anchor_path, anchor_new_side, anchor_lineno, anchor_end_lineno, + anchor_target, anchor_before, anchor_after, anchor_ctx_hash, + body, author, tour, seq, created_at, updated_at + ) VALUES ( + lower(hex(randomblob(16))), ?1, ?2, NULL, ?3, ?4, + ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12, + ?13, ?14, ?15, ?16, ?17, ?17 + ) RETURNING uid", + params![ + new.kind.as_str(), + Status::Open.as_str(), + new.changeset.name(), + new.changeset.uncommitted(), + fields.path, + fields.new_side, + fields.lineno, + fields.end_lineno, + fields.target, + fields.before, + fields.after, + fields.ctx_hash, + new.body, + new.author, + new.tour, + new.seq, + now, + ], + |row| row.get(0), + ) + .map_err(AnnotationsError::QueryFailed)?; + self.bump_revision()?; + + Ok(Annotation { + uid, + kind: new.kind, + status: Status::Open, + parent_uid: None, + changeset: new.changeset, + anchor: new.anchor, + body: new.body, + author: new.author, + tour: new.tour, + seq: new.seq, + created_at: now, + updated_at: now, + }) + } + + /// Reply to `parent_uid`. A reply carries no anchor of its own — it inherits the + /// parent's location implicitly, so re-anchoring never has to keep two copies in sync. + pub fn reply(&self, parent_uid: &str, body: &str, author: &str) -> Result { + let parent = self + .get(parent_uid)? + .ok_or_else(|| AnnotationsError::NotFound { + uid: parent_uid.to_string(), + })?; + let now = now(); + let uid: String = self + .conn + .query_row( + "INSERT INTO annotation ( + uid, kind, status, parent_uid, changeset_name, changeset_uncommitted, + body, author, created_at, updated_at + ) VALUES ( + lower(hex(randomblob(16))), ?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?8 + ) RETURNING uid", + params![ + parent.kind.as_str(), + Status::Open.as_str(), + parent_uid, + parent.changeset.name(), + parent.changeset.uncommitted(), + body, + author, + now, + ], + |row| row.get(0), + ) + .map_err(AnnotationsError::QueryFailed)?; + self.bump_revision()?; + + Ok(Annotation { + uid, + kind: parent.kind, + status: Status::Open, + parent_uid: Some(parent_uid.to_string()), + changeset: parent.changeset, + anchor: None, + body: body.to_string(), + author: author.to_string(), + tour: None, + seq: None, + created_at: now, + updated_at: now, + }) + } + + pub fn set_status(&self, uid: &str, status: Status) -> Result<()> { + let changed = self + .conn + .execute( + "UPDATE annotation SET status = ?1, updated_at = ?2 WHERE uid = ?3", + params![status.as_str(), now(), uid], + ) + .map_err(AnnotationsError::QueryFailed)?; + if changed == 0 { + return Err(AnnotationsError::NotFound { + uid: uid.to_string(), + }); + } + self.bump_revision() + } + + pub fn update_body(&self, uid: &str, body: &str) -> Result<()> { + let changed = self + .conn + .execute( + "UPDATE annotation SET body = ?1, updated_at = ?2 WHERE uid = ?3", + params![body, now(), uid], + ) + .map_err(AnnotationsError::QueryFailed)?; + if changed == 0 { + return Err(AnnotationsError::NotFound { + uid: uid.to_string(), + }); + } + self.bump_revision() + } + + /// Delete `uid` and (transitively) every reply to it. + pub fn delete(&self, uid: &str) -> Result<()> { + let tx = self + .conn + .unchecked_transaction() + .map_err(AnnotationsError::TransactionFailed)?; + + let exists: bool = tx + .query_row("SELECT 1 FROM annotation WHERE uid = ?1", [uid], |_| Ok(())) + .optional() + .map_err(AnnotationsError::QueryFailed)? + .is_some(); + if !exists { + return Err(AnnotationsError::NotFound { + uid: uid.to_string(), + }); + } + + // One level of nesting today (replies don't themselves get replies), but delete + // walks transitively in case that changes. + let mut frontier = vec![uid.to_string()]; + let mut to_delete = Vec::new(); + while let Some(id) = frontier.pop() { + let children: Vec = { + let mut stmt = tx + .prepare("SELECT uid FROM annotation WHERE parent_uid = ?1") + .map_err(AnnotationsError::QueryFailed)?; + let rows = stmt + .query_map([&id], |row| row.get(0)) + .map_err(AnnotationsError::QueryFailed)?; + rows.collect::>>() + .map_err(AnnotationsError::QueryFailed)? + }; + frontier.extend(children); + to_delete.push(id); + } + // Reverse discovery order: children were discovered after their parents, so the + // reverse walk deletes them first and never violates the parent_uid foreign key. + for id in to_delete.iter().rev() { + tx.execute("DELETE FROM annotation WHERE uid = ?1", [id]) + .map_err(AnnotationsError::QueryFailed)?; + } + bump_revision_tx(&tx)?; + tx.commit().map_err(AnnotationsError::TransactionFailed)?; + Ok(()) + } + + pub fn get(&self, uid: &str) -> Result> { + self.conn + .query_row( + "SELECT * FROM annotation WHERE uid = ?1", + [uid], + row_to_annotation, + ) + .optional() + .map_err(AnnotationsError::QueryFailed) + } + + pub fn by_changeset(&self, key: &ChangesetKey) -> Result> { + let mut stmt = self + .conn + .prepare( + "SELECT * FROM annotation WHERE changeset_name = ?1 AND changeset_uncommitted = ?2", + ) + .map_err(AnnotationsError::QueryFailed)?; + collect(stmt.query_map(params![key.name(), key.uncommitted()], row_to_annotation)) + } + + pub fn by_path(&self, key: &ChangesetKey, path: &str) -> Result> { + let mut stmt = self + .conn + .prepare( + "SELECT * FROM annotation \ + WHERE changeset_name = ?1 AND changeset_uncommitted = ?2 AND anchor_path = ?3", + ) + .map_err(AnnotationsError::QueryFailed)?; + collect(stmt.query_map( + params![key.name(), key.uncommitted(), path], + row_to_annotation, + )) + } + + /// Tour stops for `tour`, ordered by `seq`. + pub fn tour(&self, tour: &str) -> Result> { + let mut stmt = self + .conn + .prepare("SELECT * FROM annotation WHERE tour = ?1 ORDER BY seq") + .map_err(AnnotationsError::QueryFailed)?; + collect(stmt.query_map([tour], row_to_annotation)) + } + + /// The chapter annotation for `changeset`, if one exists. + pub fn chapter(&self, changeset: &ChangesetKey) -> Result> { + self.conn + .query_row( + "SELECT * FROM annotation \ + WHERE changeset_name = ?1 AND changeset_uncommitted = ?2 AND kind = ?3", + params![ + changeset.name(), + changeset.uncommitted(), + AnnotationKind::Chapter.as_str() + ], + row_to_annotation, + ) + .optional() + .map_err(AnnotationsError::QueryFailed) + } + + /// Write a whole walkthrough (chapter + ordered tour stops) in one transaction, so a + /// watcher never observes a half-authored tour. + pub fn put_walkthrough(&self, walkthrough: Walkthrough) -> Result<()> { + let tx = self + .conn + .unchecked_transaction() + .map_err(AnnotationsError::TransactionFailed)?; + let now = now(); + + if let Some(chapter) = &walkthrough.chapter { + tx.execute( + "INSERT INTO annotation ( + uid, kind, status, changeset_name, changeset_uncommitted, + body, author, created_at, updated_at + ) VALUES (lower(hex(randomblob(16))), ?1, ?2, ?3, ?4, ?5, ?6, ?7, ?7)", + params![ + AnnotationKind::Chapter.as_str(), + Status::Open.as_str(), + walkthrough.changeset.name(), + walkthrough.changeset.uncommitted(), + chapter, + walkthrough.chapter_author.as_deref().unwrap_or(""), + now, + ], + ) + .map_err(AnnotationsError::QueryFailed)?; + } + + for stop in &walkthrough.stops { + tx.execute( + "INSERT INTO annotation ( + uid, kind, status, changeset_name, changeset_uncommitted, + anchor_path, anchor_new_side, anchor_lineno, anchor_end_lineno, + anchor_target, anchor_before, anchor_after, anchor_ctx_hash, + body, author, tour, seq, created_at, updated_at + ) VALUES ( + lower(hex(randomblob(16))), ?1, ?2, ?3, ?4, + ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12, + ?13, ?14, ?15, ?16, ?17, ?17 + )", + params![ + AnnotationKind::TourStop.as_str(), + Status::Open.as_str(), + walkthrough.changeset.name(), + walkthrough.changeset.uncommitted(), + stop.anchor.path, + stop.anchor.new_side, + stop.anchor.lineno, + stop.anchor.end_lineno, + stop.anchor.target, + join_lines(&stop.anchor.before), + join_lines(&stop.anchor.after), + crate::schema::ctx_hash( + &stop.anchor.before, + &stop.anchor.target, + &stop.anchor.after + ), + stop.body, + stop.author, + walkthrough.tour, + stop.seq, + now, + ], + ) + .map_err(AnnotationsError::QueryFailed)?; + } + + bump_revision_tx(&tx)?; + tx.commit().map_err(AnnotationsError::TransactionFailed) + } + + /// A cheap fingerprint of the store's write state, for a poll-based watcher. + /// `data_version` is sqlite's `PRAGMA data_version`: it changes only when some OTHER + /// connection commits a write (this connection's own writes don't move it), so it's a + /// free echo suppression the TUI would otherwise have to hand-build. `revision` is this + /// crate's own counter, bumped on every write through this store (including this + /// connection's), for callers that want to detect their own writes too. + pub fn fingerprint(&self) -> Result { + let data_version: i64 = self + .conn + .query_row("PRAGMA data_version", [], |row| row.get(0)) + .map_err(AnnotationsError::QueryFailed)?; + let revision: i64 = self + .conn + .query_row("SELECT revision FROM meta WHERE id = 1", [], |row| { + row.get(0) + }) + .map_err(AnnotationsError::QueryFailed)?; + Ok(Fingerprint { + data_version, + revision, + }) + } + + fn bump_revision(&self) -> Result<()> { + self.conn + .execute("UPDATE meta SET revision = revision + 1 WHERE id = 1", []) + .map_err(AnnotationsError::QueryFailed)?; + Ok(()) + } +} + +fn bump_revision_tx(tx: &rusqlite::Transaction<'_>) -> Result<()> { + tx.execute("UPDATE meta SET revision = revision + 1 WHERE id = 1", []) + .map_err(AnnotationsError::QueryFailed)?; + Ok(()) +} + +fn db_path(commondir: &Path) -> PathBuf { + commondir.join("workon-review").join("annotations.db") +} + +fn now() -> i64 { + SystemTime::now() + .duration_since(UNIX_EPOCH) + .map(|d| d.as_secs() as i64) + .unwrap_or(0) +} + +fn join_lines(lines: &[String]) -> String { + lines.join("\n") +} + +fn split_lines(joined: &str) -> Vec { + if joined.is_empty() { + Vec::new() + } else { + joined.split('\n').map(str::to_string).collect() + } +} + +/// An anchor's columns, pre-materialized into owned `Option`s (all `None` for `anchor: +/// None`) so callers can splice them straight into a `params!` invocation — `params!` needs +/// each argument to own (or outlive the call) its storage, which a `match`-per-field inline +/// can't give it. +struct AnchorFields { + path: Option, + new_side: Option, + lineno: Option, + end_lineno: Option, + target: Option, + before: Option, + after: Option, + ctx_hash: Option, +} + +fn anchor_fields(anchor: Option<&Anchor>) -> AnchorFields { + match anchor { + Some(a) => AnchorFields { + path: Some(a.path.clone()), + new_side: Some(a.new_side), + lineno: Some(a.lineno), + end_lineno: Some(a.end_lineno), + target: Some(a.target.clone()), + before: Some(join_lines(&a.before)), + after: Some(join_lines(&a.after)), + ctx_hash: Some(crate::schema::ctx_hash(&a.before, &a.target, &a.after)), + }, + None => AnchorFields { + path: None, + new_side: None, + lineno: None, + end_lineno: None, + target: None, + before: None, + after: None, + ctx_hash: None, + }, + } +} + +fn collect( + rows: rusqlite::Result< + rusqlite::MappedRows<'_, impl FnMut(&Row<'_>) -> rusqlite::Result>, + >, +) -> Result> { + rows.map_err(AnnotationsError::QueryFailed)? + .collect::>>() + .map_err(AnnotationsError::QueryFailed) +} + +fn row_to_annotation(row: &Row<'_>) -> rusqlite::Result { + let kind_str: String = row.get("kind")?; + let status_str: String = row.get("status")?; + let kind = AnnotationKind::from_str(&kind_str).unwrap_or(AnnotationKind::Comment); + let status = Status::from_str(&status_str).unwrap_or(Status::Open); + + let anchor_path: Option = row.get("anchor_path")?; + let anchor = anchor_path.map(|path| -> rusqlite::Result { + Ok(Anchor { + path, + new_side: row.get("anchor_new_side")?, + lineno: row.get("anchor_lineno")?, + end_lineno: row.get("anchor_end_lineno")?, + target: row.get("anchor_target")?, + before: split_lines(&row.get::<_, String>("anchor_before")?), + after: split_lines(&row.get::<_, String>("anchor_after")?), + }) + }); + let anchor = anchor.transpose()?; + + Ok(Annotation { + uid: row.get("uid")?, + kind, + status, + parent_uid: row.get("parent_uid")?, + changeset: ChangesetKey::new( + row.get::<_, String>("changeset_name")?, + row.get("changeset_uncommitted")?, + ), + anchor, + body: row.get("body")?, + author: row.get("author")?, + tour: row.get("tour")?, + seq: row.get("seq")?, + created_at: row.get("created_at")?, + updated_at: row.get("updated_at")?, + }) +} + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::tempdir; + + fn open_temp() -> (tempfile::TempDir, AnnotationStore) { + let dir = tempdir().unwrap(); + let store = AnnotationStore::open(dir.path()).unwrap(); + (dir, store) + } + + fn sample_anchor() -> Anchor { + Anchor { + path: "src/lib.rs".into(), + new_side: true, + lineno: 10, + end_lineno: 10, + target: "fn main() {}".into(), + before: vec!["// comment".into()], + after: vec![], + } + } + + #[test] + fn schema_round_trip() { + let (_dir, store) = open_temp(); + let key = ChangesetKey::new("feature-x", false); + let inserted = store + .insert(NewAnnotation { + kind: AnnotationKind::Comment, + changeset: key.clone(), + anchor: Some(sample_anchor()), + body: "why is this here?".into(), + author: "reviewer".into(), + tour: None, + seq: None, + }) + .unwrap(); + + let fetched = store.get(&inserted.uid).unwrap().unwrap(); + assert_eq!(fetched, inserted); + assert_eq!( + fetched.anchor.unwrap().before, + vec!["// comment".to_string()] + ); + } + + #[test] + fn reply_cascade_delete() { + let (_dir, store) = open_temp(); + let key = ChangesetKey::new("feature-x", false); + let root = store + .insert(NewAnnotation { + kind: AnnotationKind::Comment, + changeset: key, + anchor: None, + body: "root".into(), + author: "a".into(), + tour: None, + seq: None, + }) + .unwrap(); + let reply = store.reply(&root.uid, "reply", "b").unwrap(); + + store.delete(&root.uid).unwrap(); + assert!(store.get(&root.uid).unwrap().is_none()); + assert!(store.get(&reply.uid).unwrap().is_none()); + } + + #[test] + fn fingerprint_unchanged_on_own_write() { + let (dir, store) = open_temp(); + let before = store.fingerprint().unwrap(); + store + .insert(NewAnnotation { + kind: AnnotationKind::Comment, + changeset: ChangesetKey::new("x", false), + anchor: None, + body: "b".into(), + author: "a".into(), + tour: None, + seq: None, + }) + .unwrap(); + let after = store.fingerprint().unwrap(); + // Own writes don't move sqlite's data_version... + assert_eq!(before.data_version, after.data_version); + // ...but this crate's own revision counter does track them. + assert_eq!(before.revision + 1, after.revision); + + // A second connection's commit DOES move data_version. + let other = AnnotationStore::open(dir.path()).unwrap(); + other + .insert(NewAnnotation { + kind: AnnotationKind::Comment, + changeset: ChangesetKey::new("x", false), + anchor: None, + body: "c".into(), + author: "a".into(), + tour: None, + seq: None, + }) + .unwrap(); + let observed = store.fingerprint().unwrap(); + assert_ne!(after.data_version, observed.data_version); + } + + #[test] + fn tour_orders_by_seq() { + let (_dir, store) = open_temp(); + store + .put_walkthrough(Walkthrough { + changeset: ChangesetKey::new("feature-x", false), + tour: "onboarding".into(), + chapter: Some("This changeset does X.".into()), + chapter_author: Some("agent".into()), + stops: vec![ + TourStop { + anchor: sample_anchor(), + body: "second".into(), + author: "agent".into(), + seq: 2, + }, + TourStop { + anchor: sample_anchor(), + body: "first".into(), + author: "agent".into(), + seq: 1, + }, + ], + }) + .unwrap(); + + let stops = store.tour("onboarding").unwrap(); + assert_eq!(stops.len(), 2); + assert_eq!(stops[0].body, "first"); + assert_eq!(stops[1].body, "second"); + + let chapter = store + .chapter(&ChangesetKey::new("feature-x", false)) + .unwrap() + .unwrap(); + assert_eq!(chapter.body, "This changeset does X."); + } + + #[test] + fn open_read_only_rejects_missing_db() { + let dir = tempdir().unwrap(); + let result = AnnotationStore::open_read_only(dir.path()); + assert!(result.is_err()); + } +} From e0eee34900ce5e7266259cfeae397df1251603f6 Mon Sep 17 00:00:00 2001 From: Eric Eldredge Date: Wed, 2 Sep 2026 12:29:31 -0400 Subject: [PATCH 2/2] docs(review): record the annotations substrate decision in ADR-039 --- docs/adr/039-review-annotations-substrate.md | 103 +++++++++++++++++++ docs/rfc/workon-review.md | 6 +- git-workon-annotations/Cargo.toml | 6 +- git-workon-annotations/src/lib.rs | 4 +- 4 files changed, 111 insertions(+), 8 deletions(-) create mode 100644 docs/adr/039-review-annotations-substrate.md diff --git a/docs/adr/039-review-annotations-substrate.md b/docs/adr/039-review-annotations-substrate.md new file mode 100644 index 00000000..f8297873 --- /dev/null +++ b/docs/adr/039-review-annotations-substrate.md @@ -0,0 +1,103 @@ +# 039: Annotations, One Substrate for Comments and Walkthroughs + +Status: accepted (2026-09-02 plan-mode interview) + +## Context + +The RFC (`docs/rfc/workon-review.md`) deferred two capabilities behind "the eventual payoff": +review comments fed back to a coding agent via MCP, and a `git workon mcp` bridge to +`git-workon-lib`'s worktree tools. Separately, the user wants an integrated version of the +`/explain-diff` skill: an agent-authored walkthrough that steps a reviewer through a stack, +changeset by changeset. Both need the same three things: a place to anchor content to a +specific line (or a whole changeset) that survives the file changing underneath it, a way for +an agent to write that content over MCP, and a way for the TUI to watch for and render it. + +Treating them as two features would mean two anchoring schemes, two stores, and two watchers +for what is structurally the same problem: attach text to a resolved location in a changeset, +author it from either the human or an agent, and keep the TUI's view live as the underlying +diff moves. + +## Decision + +**One substrate, two uses.** `AnnotationKind::{Comment, TourStop, Chapter}` share one table, +one anchoring scheme, and one store API. A walkthrough is annotations carrying a `tour` name +and `seq` order; a chapter is per-changeset prose with no line anchor. + +**Content-hash context anchoring.** An anchor stores the target line's text plus up to 3 +context lines each way. It's re-resolved every load, not on write: exact match first (stored +line number, text, and context all agree), then a windowed outward scan for the target text +scored by how much surrounding context still matches, then a whitespace-tolerant repeat of +that scan, else `Orphaned`. A failed resolution renders as "unanchored" (never silently +wrong, never crashes the view). `Orphaned` is derived per load, not persisted: only `Open` and +`Resolved` are real lifecycle states, so a discarded edit that restores the original content +un-orphans the annotation for free, with nothing to reconcile. + +**sqlite store at `/workon-review/annotations.db`.** `commondir`, not +`repo.path()` (every worktree of a repo shares one store, the same discipline +`git-workon-lib`'s graphite-metadata reader already uses for `.graphite_metadata.db`). WAL + +`busy_timeout(3000)` on the writer; `SQLITE_OPEN_READ_ONLY | SQLITE_OPEN_NO_MUTEX` for a +read-only handle. `rusqlite` (bundled) is already a workspace dependency, so this adds no new +runtime. + +**New crate `git-workon-annotations`, `publish = false` + `[package.metadata.dist] dist = +false`.** Same posture ADR-033 set for `git-workon-review`: a scaffold-stage crate stays out +of release-plz's auto-publish and cargo-dist's auto-bin-inclusion until it's actually ready to +ship, and `dist = false` is the explicit tripwire for that later flip so removing `publish = +false` alone can't silently start shipping an undesigned binary. Dependencies: `rusqlite`, +`thiserror`, `miette`. Serde-free (plain structs with rusqlite row mapping): the store is a +lib, `git-workon-mcp` (ADR-040) is its second consumer, and the serde-free/git2-free posture +exists so that consumer owns the JSON boundary. No `git2` dependency: the store takes a +`commondir: &Path` the caller resolves, so this crate never needs to open a repository itself. + +**Why a new crate at all, when the RFC says "no separate core crate until a second consumer +exists."** That condition is now met. `git workon mcp` is a second consumer of the same +comment/annotation data the TUI reads and writes (a lib both `git-workon-review` and, via +its MCP binary, `git-workon` need). This is exactly the fork the RFC's Agent-loop bullet left +open. Putting the store in `git-workon-review`'s own lib would mean either the CLI depends on +the review crate's whole diff/render surface just to reach a sqlite table, or the annotation +code gets duplicated. A dedicated crate is the smaller dependency edge either way. + +**MCP lives in its own crate, `git-workon-mcp` (ADR-040), this store's second consumer.** +This crate stays a lib with no bin target and no `rmcp`/tokio dependency; `git-workon-mcp` +depends on it, owns the JSON boundary, and is reached from `git-workon` via the existing +external-subcommand PATH dispatch (`git-workon/src/dispatch.rs`), not a compile-time +dependency or a built-in `Cmd::Mcp`. ADR-040 covers the crate split, the publish-blocker +reasoning for keeping it off the `git-workon` dependency graph, and the transport choice +(`rmcp` 3.2, minimal features, current-thread tokio). + +**Gutter marker, not an edge glyph, for the TUI's annotation indicator (deferred to the read +slice, recorded here for continuity).** Both content edges of a diff row are already claimed +by horizontal-scroll affordances; the gutter's trailing space survives panning, so that's +where the marker goes. + +## Consequences + +- Comments and walkthrough stops are the same row shape, so the TUI's marker index, overlay, + and store watcher are built once and serve both, instead of twice. +- The anchoring scheme is a genuine trade: it can misplace an annotation on a heavily + rewritten line (context match is a heuristic, not a guarantee), but it never crashes or + silently attaches to the wrong line without saying so: an unresolved anchor always renders + as `Orphaned`, visibly. +- The store is `rusqlite::Connection`, which is `!Sync`: every consumer (the TUI's + event-loop thread, each MCP tool call) must own or briefly borrow its own connection, and no + connection crosses a thread boundary as shared state. +- `git-workon-annotations` has no bin target and never will: `git-workon-mcp` (ADR-040) + depends on it as a lib, the same relationship `git-workon-review`'s TUI has to it. +- The initial-publish flip for this crate follows the same three steps ADR-033 lists for + `git-workon-review`: drop `publish = false`, add the crate to `release-plz.toml` with no + shared `version_group`, and decide `dist = false`'s fate deliberately rather than by + omission. `git-workon-mcp`'s own distribution (a second binary through cargo-dist and the + homebrew formula patch step) is a separate decision ADR-033 already flagged as unsolved for + a second binary generally; this ADR doesn't resolve it. + +## References + +- `docs/rfc/workon-review.md`: Agent-loop bullet and Comments decision row, updated + alongside this ADR +- [ADR-008](008-error-handling-strategy.md): the two-layer error pattern this crate's + `error.rs` follows +- [ADR-033](033-review-crate-workspace-placement.md): the `publish = false` / `dist = false` + scaffold posture this crate adopts, and the second-binary distribution gap it already + flagged +- `git-workon-lib/src/stack/graphite.rs`: the `commondir`-based, `READ_ONLY | NO_MUTEX` + sqlite reader this store's open/open_read_only follow diff --git a/docs/rfc/workon-review.md b/docs/rfc/workon-review.md index 97fdd672..fc101b28 100644 --- a/docs/rfc/workon-review.md +++ b/docs/rfc/workon-review.md @@ -15,7 +15,7 @@ It is the productization of a working Neovim prototype (`~/.config/nvim/lua/app/ |---|---| | Positioning | Changeset review tool; not a lazygit competitor. Comments-to-agent is a first-class capability, not a stretch. **Reprioritized 2026-07-08 (direction B):** near-term goal is the author's own daily diff-review + git driver; the agent-loop/comments become the eventual payoff, not the next work. See "Roadmap reprioritized" under Milestones. | | Home | This workspace, as sibling crate `git-workon-review`. | -| Crate layout | ONE crate, lib+bin targets. lib = review domain (diff parse, word-diff, staging, changeset views); bin = TUI + `mcp` subcommand. No separate core crate until a second consumer exists. | +| Crate layout | `git-workon-review`: lib+bin targets, lib = review domain (diff parse, word-diff, staging, changeset views), bin = TUI. **Superseded 2026-09-02 (ADR-039):** the "no separate core crate until a second consumer exists" condition is now met — `git workon mcp` is a second consumer of the annotation store (see the Agent-loop bullet under Milestones). The store lives in its own crate, `git-workon-annotations`, `publish = false` like `git-workon-review` itself; `git workon mcp` is served from its own `git-workon-mcp` crate (ADR-040), reached from the published `git-workon` binary via the existing external-subcommand PATH dispatch rather than a compile-time dependency (the publish blocker: a published crate can't depend on a `publish = false` one). | | Name | Package == binary == `git-workon-review`. `git workon-review` works via git's native `git-*` dispatch. (`git-review` is squatted on crates.io + Gerrit-loaded; `docket` too docker-adjacent; bare `review` superseded by suite framing; `signoff` was the free runner-up.) | | `git-workon review` dispatch | `git-workon` adds cargo-style external-subcommand dispatch: unknown subcommand → exec `git-workon-` on PATH, args passed through. | | NO `workon` binary | Deliberate: Python virtualenvwrapper keeps the `workon` name. Do not re-propose. | @@ -26,7 +26,7 @@ It is the productization of a working Neovim prototype (`~/.config/nvim/lua/app/ | Highlighting | tree-sitter (tree-sitter-highlight), syntect as long-tail fallback. Measured: ts ~0.01ms/line vs syntect ~0.19ms/line, and better output. Grammar set + gotchas are in the spike. | | View model | Full parity with the prototype's four zoom states (split/combined/unstaged/staged + attributed rendering). If v1 must shrink, cut zoom states — never the comments loop. | | v1 sources | uncommitted, stack, ref/range, **PR** — all folded into **the source-selector work ("review any source")** (PR was deferred; now first, via git-workon-lib's `pr.rs`). | -| Comments | MCP: on-disk comment store (`.review/` JSON or sqlite) + `git-workon-review mcp` stdio subcommand serving get/resolve tools; TUI watches the store. Degrades to a plain file convention for non-MCP harnesses. | +| Comments | **Superseded 2026-09-02 (ADR-039):** comments are one `AnnotationKind` in the shared annotations substrate (see the Agent-loop bullet under Milestones), not a standalone `.review/` store. sqlite at `/workon-review/annotations.db`, content-hash context anchoring (re-resolved per load; a stale anchor renders `Orphaned`, never wrong), served over MCP by `git workon mcp` (`git-workon-mcp`, its own crate — ADR-040 — dispatched from `git-workon` over PATH, not a `git-workon-review mcp` subcommand). TUI watches the store via `PRAGMA data_version`. | | Edit flow | Embedded: `nvim --server $NVIM --remote + `. Standalone: `$EDITOR`. File watcher refreshes on save. | | Completions | Full clap_complete (unstable-dynamic, already a workspace dep) on the direct binary. Work item: git-workon's dynamic completer enumerates `git-workon-*` on PATH and delegates post-subcommand completion via `COMPLETE= git-workon-review -- `. Git-level shims: on demand only. | | Study first | `jjr` crate (agent jj-stack review surface), `triage-tui`, `wb300` — adjacent tools found during naming research. | @@ -160,7 +160,7 @@ The remaining roadmap is resequenced around the tool being **the author's own ev - **Conflict resolution** *(stretch)*. Resolve merge/rebase conflicts in the SBS view. Large surface; may not make v1. -- **Agent loop** *(the eventual north star)*. On-disk comment store keyed to `(changeset_id, path, side, lnum)` with a rebase-survival anchoring strategy + TUI comment UX (create/view/resolve, store-watch refresh), and a **unified `git workon mcp`** stdio server bridging git-workon-lib worktree tools (`agent-integration.md` Model C) *and* the comment store. **Open forks (unchanged, resolve at design time):** comment-store home — a lib both the review crate and `git-workon` depend on, since `git workon mcp` is a second consumer (reopens the "no separate core crate" decision); the anchoring strategy; MCP crate/transport (`rmcp` vs hand-rolled JSON-RPC-over-stdio). Deferred behind the daily-driver work. +- **Agent loop** *(the eventual north star)*. **Design locked 2026-09-02 (ADR-039); the three open forks below are resolved, not just re-flagged.** One substrate serves both review comments and an integrated `/explain-diff`-style walkthrough: `AnnotationKind::{Comment, TourStop, Chapter}` in one sqlite table at `/workon-review/annotations.db`, anchored by content-hash context (target line + 3 lines each way, re-resolved per load: exact match, then a scored windowed scan, then whitespace-tolerant, else `Orphaned` — never silently wrong) rather than the originally-proposed `(changeset_id, path, side, lnum)` key. **Comment-store home:** its own crate, `git-workon-annotations` (`publish = false`, serde-free, no git2) — `git workon mcp` is the second consumer the "no separate core crate" rule was waiting on, so that condition is now met and the rule no longer applies to it. **MCP crate/transport:** `rmcp` 3.2 (minimal features, current-thread tokio), not hand-rolled JSON-RPC, served from its own `git-workon-mcp` crate (ADR-040) and reached from `git-workon` via the existing PATH-dispatch mechanism (not a built-in `Cmd::Mcp`, and not a `git-workon-review mcp` subcommand) — this is what lets the published `git-workon` binary depend on the feature without depending on the unpublished annotations crate. Landing as five stacked slices (crate scaffold → TUI read → TUI authoring → prose/walkthrough polish → MCP crate); each lands alone. Still deferred behind the daily-driver work, but no longer blocked on open design questions. ## Orchestration notes diff --git a/git-workon-annotations/Cargo.toml b/git-workon-annotations/Cargo.toml index dbe26701..ed2ffd12 100644 --- a/git-workon-annotations/Cargo.toml +++ b/git-workon-annotations/Cargo.toml @@ -15,9 +15,9 @@ include = [ "Cargo.toml", "LICENSE*", ] -# Not yet published: this crate has no bin target yet (the MCP server lands in a later -# slice) and the store schema is still settling. Follow the ADR-033 posture: flipping to -# publish is a deferred sub-decision, not an oversight to fix later. +# Not yet published: this crate has no bin target (the MCP server is its own crate, +# git-workon-mcp — ADR-040) and the store schema is still settling. Follow the ADR-033 +# posture: flipping to publish is a deferred sub-decision, not an oversight to fix later. publish = false [lib] diff --git a/git-workon-annotations/src/lib.rs b/git-workon-annotations/src/lib.rs index 3d27e031..665c8aad 100644 --- a/git-workon-annotations/src/lib.rs +++ b/git-workon-annotations/src/lib.rs @@ -8,8 +8,8 @@ //! //! This crate is serde-free and git2-free: [`store::AnnotationStore`] takes a `commondir` //! path (the caller resolves it, e.g. via `git2::Repository::commondir()`), and the types -//! here are plain structs with rusqlite row mapping. JSON only enters at the MCP boundary -//! (a later slice, hosted as a second bin target in this crate — see ADR-039). +//! here are plain structs with rusqlite row mapping. JSON only enters at the MCP boundary, +//! owned by `git-workon-mcp`, this crate's second consumer (see ADR-040). //! //! ## Status //!