forge_primitives/
review.rs

1//! Durable local-review wire types.
2//!
3//! A review thread is a comment anchored to a diff line in the `/diff`
4//! overlay, persisted per `(project, branch)` so it survives overlay
5//! close and forge restarts. Pure data shapes; the redb persistence
6//! lives in `forge-workspace::store::review` and the re-anchoring in
7//! `forge-agent::env::git_diff`.
8
9use serde::{Deserialize, Serialize};
10
11use crate::session_slot::SessionSlot;
12
13/// Lifecycle state of a review thread. A worker reply flips `Open` ->
14/// `Addressed`; only the user ever `Resolved`s; `Outdated` marks a
15/// thread whose anchored line drifted out from under it.
16#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)]
17pub enum ReviewStatus {
18    Open,
19    Addressed,
20    Resolved,
21    Outdated,
22}
23
24/// Which side of the diff the anchored line sits on.
25#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Serialize, Deserialize)]
26pub enum ReviewSide {
27    Old,
28    New,
29}
30
31/// Who wrote a review comment.
32#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
33pub enum ReviewAuthor {
34    User,
35    Agent { label: String },
36}
37
38/// One comment in a thread; `at` is an rfc3339 timestamp. `review_id` is
39/// the [`ReviewSet`] that sealed this turn (`None` = not yet filed);
40/// `#[serde(default)]` loads turns stored before membership moved onto
41/// them. Only user turns are ever filed - an agent reply answers a round
42/// rather than forming one.
43#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
44pub struct ReviewComment {
45    pub author: ReviewAuthor,
46    pub text: String,
47    pub at: String,
48    #[serde(default)]
49    pub review_id: Option<String>,
50}
51
52/// Durable, drift-robust location of a review thread, re-resolved to a
53/// positional index against a fresh diff scan on every overlay open;
54/// `content_hash` + `context` let the resolver re-anchor a moved line
55/// or flag it `Outdated`.
56#[derive(Clone, Debug, PartialEq, Eq, Deserialize)]
57pub struct ReviewAnchor {
58    pub path: String,
59    pub side: ReviewSide,
60    pub line: u32,
61    /// The anchored line's hash, as a full u64.
62    ///
63    /// It crosses as decimal digits rather than a JSON number, which is
64    /// the convention protobuf's JSON mapping gives its 64-bit fields: the
65    /// web client reads a thread's hash and re-sends it on every reply,
66    /// and JS numbers are doubles - exact only to 2^53 - so a number form
67    /// would come back rounded and silently anchor the thread elsewhere.
68    /// The deserializer still accepts a number, which is what blobs
69    /// written before this change hold.
70    #[serde(deserialize_with = "u64_from_number_or_string")]
71    pub content_hash: u64,
72    pub context: Vec<String>,
73    pub base_ref: String,
74}
75
76/// Hand-written for one field's sake: `content_hash` goes out as digits,
77/// and `serde`'s `serialize_with` would hand the helper a `&u64` - a
78/// signature the pedantic lints refuse, and this project carries no
79/// suppression for it. The deserializer stays derived above.
80impl Serialize for ReviewAnchor {
81    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
82    where
83        S: serde::Serializer,
84    {
85        #[derive(Serialize)]
86        struct Wire<'a> {
87            path: &'a str,
88            side: ReviewSide,
89            line: u32,
90            content_hash: String,
91            context: &'a [String],
92            base_ref: &'a str,
93        }
94        Wire {
95            path: &self.path,
96            side: self.side,
97            line: self.line,
98            content_hash: self.content_hash.to_string(),
99            context: &self.context,
100            base_ref: &self.base_ref,
101        }
102        .serialize(serializer)
103    }
104}
105
106/// Accept a u64 as a JSON number or as its decimal string; anything else is
107/// refused rather than rounded.
108fn u64_from_number_or_string<'de, D>(deserializer: D) -> Result<u64, D::Error>
109where
110    D: serde::Deserializer<'de>,
111{
112    #[derive(serde::Deserialize)]
113    #[serde(untagged)]
114    enum Either {
115        Number(u64),
116        Digits(String),
117    }
118    match Either::deserialize(deserializer)? {
119        Either::Number(value) => Ok(value),
120        Either::Digits(digits) => digits
121            .parse()
122            .map_err(|_| serde::de::Error::custom(format!("`{digits}` is not a u64 in decimal"))),
123    }
124}
125
126/// A persisted review thread: an anchor plus its comment chain and
127/// lifecycle state; `created_at` / `updated_at` are rfc3339. `commit` is
128/// the sha the thread was authored under (`None` = whole-diff scope);
129/// `#[serde(default)]` loads pre-scope threads as whole-diff. Review
130/// membership lives per-turn on [`ReviewComment::review_id`], so one
131/// thread can span several rounds of a conversation.
132#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
133pub struct ReviewThread {
134    pub id: String,
135    pub anchor: ReviewAnchor,
136    pub comments: Vec<ReviewComment>,
137    pub status: ReviewStatus,
138    pub created_at: String,
139    pub updated_at: String,
140    #[serde(default)]
141    pub commit: Option<String>,
142}
143
144impl ReviewThread {
145    /// Whether any user turn is filed into `review_id`, i.e. whether this
146    /// thread is one of that review's comments.
147    pub fn is_in_review(&self, review_id: &str) -> bool {
148        self.comments.iter().any(|c| c.review_id.as_deref() == Some(review_id))
149    }
150
151    /// The review this thread first appeared in, driving the card's stable
152    /// `R#` tag. `None` while nothing is filed yet.
153    pub fn origin_review(&self) -> Option<&str> {
154        self.comments.iter().find_map(|c| c.review_id.as_deref())
155    }
156
157    /// The most recent review the thread has a turn in - the round a
158    /// re-nudge is about.
159    pub fn latest_review(&self) -> Option<&str> {
160        self.comments.iter().rev().find_map(|c| c.review_id.as_deref())
161    }
162
163    /// Whether a worker's answer on this thread is still waiting on the
164    /// reviewer. `Open` is excluded even when an agent turn is last: that
165    /// state is only reached by a reopen, which is the reviewer handing
166    /// the work back.
167    pub fn awaits_reviewer(&self) -> bool {
168        matches!(self.status, ReviewStatus::Addressed | ReviewStatus::Outdated)
169            && matches!(self.comments.last(), Some(c) if matches!(c.author, ReviewAuthor::Agent { .. }))
170    }
171
172    /// Whether the user has written a turn no review has sealed yet. This
173    /// is what makes a thread submittable, whether it is a fresh comment
174    /// or a reply on a thread already filed into an earlier review.
175    pub fn has_unfiled_user_turn(&self) -> bool {
176        self.comments
177            .iter()
178            .any(|c| matches!(c.author, ReviewAuthor::User) && c.review_id.is_none())
179    }
180}
181
182/// A submitted review: the sealed grouping of the comments filed under
183/// one pass, plus an optional overview. Persisted per `(project, branch)`.
184/// `number` is the human-facing 1-based sequence (`R1`, `R2`, ...) minted
185/// as the branch's review count + 1; `id` is the stable key turns carry in
186/// [`ReviewComment::review_id`]. `created_at` is rfc3339.
187#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
188pub struct ReviewSet {
189    pub id: String,
190    pub number: u32,
191    pub summary: Option<String>,
192    pub created_at: String,
193}
194
195/// A doc-scoped thread's scope: `base_ref` is this prefix plus the
196/// document's resolved absolute path, so the scope key a thread set is
197/// stored under IS the document.
198pub const DOC_SCOPE_PREFIX: &str = "doc:";
199
200/// The scope ref a document's threads are stored under.
201pub fn doc_scope_ref(path: &str) -> String {
202    format!("{DOC_SCOPE_PREFIX}{path}")
203}
204
205/// Whether a scope ref spells a document rather than a branch. **The one
206/// predicate**: the readers that place threads against a diff and the
207/// branch sweep all ask this, so a doc scope is skipped in exactly one
208/// place. It takes the ref string, not the anchor, because the sweep has
209/// only the stored scope in hand.
210pub fn is_doc_scope(base_ref: &str) -> bool {
211    base_ref.starts_with(DOC_SCOPE_PREFIX)
212}
213
214/// The document's resolved path a doc scope ref carries, or `None` for a
215/// branch scope.
216pub fn doc_scope_path(base_ref: &str) -> Option<&str> {
217    base_ref.strip_prefix(DOC_SCOPE_PREFIX)
218}
219
220/// A tree scope's scope prefix: a seat's own tree as the scope key, so the
221/// files page's comments follow the SEAT (a worker's worktree, the lead's
222/// checkout) rather than any branch the tree happens to be on.
223pub const TREE_SCOPE_PREFIX: &str = "tree:";
224
225/// The scope ref a seat's tree's threads are stored under. Built from the
226/// slot's accessors, never from `display()`: that string is for reading,
227/// not for parsing or keying.
228pub fn tree_scope_ref(slot: &SessionSlot) -> String {
229    format!("{TREE_SCOPE_PREFIX}{}/{}/{}", slot.org(), slot.project(), slot.label())
230}
231
232/// Whether a scope ref spells a seat's tree rather than a branch. **The one
233/// predicate**, like its doc-scope sibling: the branch sweep and any reader
234/// that must skip tree scopes all ask this.
235pub fn is_tree_scope(base_ref: &str) -> bool {
236    base_ref.starts_with(TREE_SCOPE_PREFIX)
237}
238
239#[cfg(test)]
240mod tests {
241    use super::*;
242    use crate::session_slot::SessionSlot;
243
244    /// The tree scope: a ref that spells a SEAT's tree, not a branch. The
245    /// branch named `trees/rewrite` is the control - it starts with the same
246    /// letters and is still not a tree scope - and so is the bare prefix
247    /// without its colon.
248    #[test]
249    fn a_tree_scope_ref_is_recognised_and_carries_its_seat() {
250        let lead = SessionSlot::lead("Busytools", "forge");
251        assert_eq!(tree_scope_ref(&lead), "tree:Busytools/forge/lead");
252        assert!(is_tree_scope(&tree_scope_ref(&lead)));
253
254        let worker = SessionSlot::worker("Busytools", "forge", "tree-review-survey");
255        assert_eq!(tree_scope_ref(&worker), "tree:Busytools/forge/tree-review-survey");
256
257        assert!(!is_tree_scope("trees/rewrite"), "a branch is not a tree scope");
258        assert!(!is_tree_scope("tree"), "the bare prefix is not one");
259        assert!(!is_tree_scope("doc:/proj/docs/plan.md"), "nor is a document");
260        assert!(!is_tree_scope(""), "nor is nothing a scope");
261    }
262
263    /// The doc scope: a ref that spells a document, not a branch. The
264    /// branch named `docs/fix` is the control - it starts with the same
265    /// letters and is still not a doc scope.
266    #[test]
267    fn a_doc_scope_ref_is_recognised_and_carries_its_path() {
268        assert_eq!(doc_scope_ref("/proj/docs/plan.md"), "doc:/proj/docs/plan.md");
269        assert!(is_doc_scope("doc:/proj/docs/plan.md"));
270        assert_eq!(doc_scope_path("doc:/proj/docs/plan.md"), Some("/proj/docs/plan.md"));
271
272        assert!(!is_doc_scope("docs/fix"), "a branch is not a doc scope");
273        assert!(!is_doc_scope("main"));
274        assert!(!is_doc_scope(""), "nor is nothing a scope");
275        assert_eq!(doc_scope_path("main"), None);
276    }
277
278    /// The anchor's hash crosses as decimal digits, and a JSON number is
279    /// still accepted - which is what blobs written before the digits form
280    /// hold - while anything else is refused rather than rounded: a hash
281    /// silently wrong anchors a thread to someone else's line.
282    #[test]
283    fn an_anchor_hash_crosses_as_decimal_digits() {
284        let mut thread = sample_thread();
285        thread.anchor.content_hash = u64::MAX;
286        let encoded = serde_json::to_value(&thread).expect("encodes");
287        assert_eq!(
288            encoded["anchor"]["content_hash"],
289            serde_json::json!("18446744073709551615"),
290            "serialization is the digits, which JS can carry exactly",
291        );
292        let round_tripped: ReviewThread =
293            serde_json::from_value(encoded.clone()).expect("digits decode");
294        assert_eq!(round_tripped.anchor.content_hash, u64::MAX);
295
296        let mut legacy = encoded.clone();
297        legacy["anchor"]["content_hash"] = serde_json::json!(u64::MAX);
298        let decoded: ReviewThread =
299            serde_json::from_value(legacy).expect("a stored number still decodes");
300        assert_eq!(
301            decoded.anchor.content_hash,
302            u64::MAX,
303            "so blobs written before the digits form need no migration",
304        );
305
306        let mut bad = encoded;
307        bad["anchor"]["content_hash"] = serde_json::json!("twelve");
308        assert!(
309            serde_json::from_value::<ReviewThread>(bad).is_err(),
310            "a non-decimal string is refused rather than rounded",
311        );
312    }
313
314    fn sample_thread() -> ReviewThread {
315        ReviewThread {
316            id: "thread-1".to_owned(),
317            anchor: ReviewAnchor {
318                path: "crates/forge-workspace/src/workspace.rs".to_owned(),
319                side: ReviewSide::New,
320                line: 4781,
321                content_hash: 0xdead_beef_cafe_f00d,
322                context: vec![
323                    "prev line".to_owned(),
324                    "anchored".to_owned(),
325                    "next line".to_owned(),
326                ],
327                base_ref: "main".to_owned(),
328            },
329            comments: vec![
330                ReviewComment {
331                    author: ReviewAuthor::User,
332                    text: "does this fire for the failure-notice path too?".to_owned(),
333                    at: "2026-07-19T10:00:00Z".to_owned(),
334                    review_id: None,
335                },
336                ReviewComment {
337                    author: ReviewAuthor::Agent { label: "implementer".to_owned() },
338                    text: "yes, test added".to_owned(),
339                    at: "2026-07-19T10:05:00Z".to_owned(),
340                    review_id: None,
341                },
342            ],
343            status: ReviewStatus::Resolved,
344            created_at: "2026-07-19T10:00:00Z".to_owned(),
345            updated_at: "2026-07-19T10:05:00Z".to_owned(),
346            commit: None,
347        }
348    }
349
350    #[test]
351    fn review_thread_round_trips_through_json() {
352        let thread = sample_thread();
353        let json = serde_json::to_string(&thread).expect("serialize");
354        let back: ReviewThread = serde_json::from_str(&json).expect("deserialize");
355        assert_eq!(thread, back);
356    }
357
358    #[test]
359    fn large_content_hash_survives_json_round_trip() {
360        // u64 above 2^53 must not lose precision through serde_json.
361        let mut thread = sample_thread();
362        thread.anchor.content_hash = u64::MAX;
363        let json = serde_json::to_string(&thread).expect("serialize");
364        let back: ReviewThread = serde_json::from_str(&json).expect("deserialize");
365        assert_eq!(back.anchor.content_hash, u64::MAX);
366    }
367
368    #[test]
369    fn commit_scoped_thread_round_trips() {
370        let mut thread = sample_thread();
371        thread.commit = Some("abc123".to_owned());
372        let json = serde_json::to_string(&thread).expect("serialize");
373        let back: ReviewThread = serde_json::from_str(&json).expect("deserialize");
374        assert_eq!(back.commit, Some("abc123".to_owned()));
375    }
376
377    #[test]
378    fn thread_without_commit_key_defaults_to_none() {
379        // A whole-diff thread persisted before the scope field existed has
380        // no `commit` key on disk; it must load as `None`, not error.
381        let json = r#"{
382            "id": "t1",
383            "anchor": {
384                "path": "a.rs",
385                "side": "New",
386                "line": 1,
387                "content_hash": 0,
388                "context": [],
389                "base_ref": "main"
390            },
391            "comments": [],
392            "status": "Open",
393            "created_at": "2026-07-19T10:00:00Z",
394            "updated_at": "2026-07-19T10:00:00Z"
395        }"#;
396        let back: ReviewThread = serde_json::from_str(json).expect("deserialize legacy thread");
397        assert_eq!(back.commit, None);
398    }
399
400    #[test]
401    fn turn_review_id_defaults_to_none_when_absent() {
402        // A turn persisted before membership moved onto turns has no
403        // `review_id` key on disk; it must load as unfiled, not error. The
404        // thread-level `review_id` such a row also carries is ignored here
405        // and lifted by the store on decode.
406        let json = r#"{
407            "id": "t1",
408            "anchor": {
409                "path": "a.rs",
410                "side": "New",
411                "line": 1,
412                "content_hash": 0,
413                "context": [],
414                "base_ref": "main"
415            },
416            "comments": [
417                { "author": "User", "text": "hmm", "at": "2026-07-19T10:00:00Z" }
418            ],
419            "status": "Open",
420            "created_at": "2026-07-19T10:00:00Z",
421            "updated_at": "2026-07-19T10:00:00Z",
422            "review_id": "review-3"
423        }"#;
424        let back: ReviewThread = serde_json::from_str(json).expect("deserialize pre-turn thread");
425        assert_eq!(back.comments[0].review_id, None);
426    }
427
428    #[test]
429    fn filed_turn_round_trips_its_review_id() {
430        let mut thread = sample_thread();
431        thread.comments[0].review_id = Some("review-7".to_owned());
432        let json = serde_json::to_string(&thread).expect("serialize");
433        let back: ReviewThread = serde_json::from_str(&json).expect("deserialize");
434        assert_eq!(back.comments[0].review_id, Some("review-7".to_owned()));
435    }
436
437    #[test]
438    fn membership_spans_every_review_the_turns_were_sealed_into() {
439        let mut thread = sample_thread();
440        thread.comments[0].review_id = Some("review-1".to_owned());
441        thread.comments.push(ReviewComment {
442            author: ReviewAuthor::User,
443            text: "still not right".to_owned(),
444            at: "2026-07-19T11:00:00Z".to_owned(),
445            review_id: Some("review-2".to_owned()),
446        });
447        assert!(thread.is_in_review("review-1"));
448        assert!(thread.is_in_review("review-2"));
449        assert!(!thread.is_in_review("review-3"));
450        assert_eq!(thread.origin_review(), Some("review-1"), "origin is the earliest filed turn");
451        assert_eq!(thread.latest_review(), Some("review-2"));
452        assert!(!thread.has_unfiled_user_turn(), "every user turn is sealed");
453    }
454
455    #[test]
456    fn an_unsealed_reply_makes_the_thread_submittable_again() {
457        let mut thread = sample_thread();
458        thread.comments[0].review_id = Some("review-1".to_owned());
459        assert!(!thread.has_unfiled_user_turn());
460        thread.comments.push(ReviewComment {
461            author: ReviewAuthor::User,
462            text: "reopening this".to_owned(),
463            at: "2026-07-19T11:00:00Z".to_owned(),
464            review_id: None,
465        });
466        assert!(thread.has_unfiled_user_turn(), "the new turn is unfiled work");
467        assert_eq!(thread.origin_review(), Some("review-1"), "origin is unaffected by a new turn");
468    }
469
470    #[test]
471    fn an_unfiled_agent_reply_is_not_submittable_work() {
472        // Only user turns form a review round; an agent reply never carries
473        // membership and must not make the thread look submittable.
474        let mut thread = sample_thread();
475        thread.comments[0].review_id = Some("review-1".to_owned());
476        assert!(!thread.has_unfiled_user_turn());
477        assert_eq!(thread.comments[1].review_id, None, "the agent turn stays unfiled");
478    }
479
480    #[test]
481    fn an_agent_reply_on_an_addressed_thread_awaits_the_reviewer() {
482        let mut thread = sample_thread();
483        thread.status = ReviewStatus::Addressed;
484        assert!(thread.awaits_reviewer(), "a worker answered and nobody has acted since");
485
486        // A drifted anchor doesn't make the answer read.
487        thread.status = ReviewStatus::Outdated;
488        assert!(thread.awaits_reviewer());
489    }
490
491    #[test]
492    fn a_reviewer_turn_stops_the_thread_awaiting() {
493        let mut thread = sample_thread();
494        thread.status = ReviewStatus::Addressed;
495        thread.comments.push(ReviewComment {
496            author: ReviewAuthor::User,
497            text: "not quite".to_owned(),
498            at: "2026-07-19T11:00:00Z".to_owned(),
499            review_id: None,
500        });
501        assert!(
502            !thread.awaits_reviewer(),
503            "the reviewer replied - the ball is back with the worker"
504        );
505    }
506
507    #[test]
508    fn resolving_or_reopening_stops_the_thread_awaiting() {
509        let mut thread = sample_thread();
510        thread.status = ReviewStatus::Resolved;
511        assert!(!thread.awaits_reviewer(), "resolve is how a read-and-accepted answer clears");
512
513        // Reopen leaves the agent turn last but hands the work back.
514        thread.status = ReviewStatus::Open;
515        assert!(!thread.awaits_reviewer(), "a reopened thread is the worker's again");
516    }
517
518    #[test]
519    fn a_thread_nobody_answered_does_not_await_the_reviewer() {
520        let mut thread = sample_thread();
521        thread.comments.truncate(1);
522        thread.status = ReviewStatus::Open;
523        assert!(!thread.awaits_reviewer(), "no agent turn, nothing to read");
524    }
525
526    #[test]
527    fn review_set_round_trips_through_json() {
528        let review = ReviewSet {
529            id: "review-1".to_owned(),
530            number: 3,
531            summary: Some("Two nits on error handling.".to_owned()),
532            created_at: "2026-07-23T10:00:00Z".to_owned(),
533        };
534        let json = serde_json::to_string(&review).expect("serialize");
535        let back: ReviewSet = serde_json::from_str(&json).expect("deserialize");
536        assert_eq!(review, back);
537    }
538
539    #[test]
540    fn review_set_without_summary_round_trips() {
541        let review = ReviewSet {
542            id: "review-2".to_owned(),
543            number: 1,
544            summary: None,
545            created_at: "2026-07-23T10:00:00Z".to_owned(),
546        };
547        let json = serde_json::to_string(&review).expect("serialize");
548        let back: ReviewSet = serde_json::from_str(&json).expect("deserialize");
549        assert_eq!(back.summary, None);
550    }
551
552    #[test]
553    fn all_status_variants_round_trip() {
554        for status in [
555            ReviewStatus::Open,
556            ReviewStatus::Addressed,
557            ReviewStatus::Resolved,
558            ReviewStatus::Outdated,
559        ] {
560            let json = serde_json::to_string(&status).expect("serialize");
561            let back: ReviewStatus = serde_json::from_str(&json).expect("deserialize");
562            assert_eq!(status, back);
563        }
564    }
565
566    #[test]
567    fn old_side_and_user_author_round_trip() {
568        let anchor = ReviewAnchor {
569            path: "a.rs".to_owned(),
570            side: ReviewSide::Old,
571            line: 12,
572            content_hash: 7,
573            context: Vec::new(),
574            base_ref: "HEAD".to_owned(),
575        };
576        let json = serde_json::to_string(&anchor).expect("serialize");
577        let back: ReviewAnchor = serde_json::from_str(&json).expect("deserialize");
578        assert_eq!(anchor, back);
579        assert_eq!(back.side, ReviewSide::Old);
580    }
581}
forge_primitives/review.rs

forge_primitives/
review.rs
