git ents: model: migrate the forge and surface crates off uuid onto derived identity

commit b1e29b9

model: migrate the forge and surface crates off uuid onto derived identity

The kernel (phase-10-kernel) already recomputes every meta-ref name from its signed content; this closes the loop on the crates that write those refs. A comment’s and an issue’s id becomes the oid of its own genesis commit (sign-then-name via ents_receive::propose_genesis), never a locally minted uuid. A review moves to the composite reviews/<target>/<member> key ents-model already exposes: its reviewed- commit field is renamed target to mirror ResultRecord/Redaction’s own accessor convention, and re-reviewing a commit whose ancestor this member already reviewed advances the same two refs (entity + retention pin) fast-forward rather than minting an unrelated review (model.review-pin). git-ents and ents-web pick up Member’s new id field, Effect’s new name field, and read results back as ResultRecord instead of a bare Status. Porcelain and web abbreviate a comment/issue id to seven hex characters for display, the way git abbreviates a commit oid; full ids remain in refnames and machine-readable (--porcelain) output.

feat: add ents_forge::abbreviate_id for git-style short-oid display feat: add review::new’s fast-forward re-review path via ancestor lookup feat: resolve a review’s own member id from the signer’s enrolled key, falling back to a fingerprint placeholder when unenrolled (mirrors `git_ents::commands::serve::build_state’s identical fallback) remove: `uuid from the workspace — no crate mints an id anymore Assisted-by: Claude:claude-sonnet-5

Joseph D. Carpinelli · 1 month ago

tree browse at HEAD · parents: eb79d4a · comment on this commit

Reviews

No reviews of this commit yet — record a verdict below.

Start a review

Cargo.lock @@ -1139,7 +1139,6 @@ "rstest", "tempfile", "thiserror 2.0.18", - "uuid", ] [[package]] @@ -1757,7 +1756,6 @@ "thiserror 2.0.18", "tokio", "tower", - "uuid", ] [[package]] @@ -4738,17 +4736,6 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b6c140620e7ffbb22c2dee59cafe6084a59b5ffc27a8859a5f0d494b5d52b6be" -[[package]] -name = "uuid" -version = "1.23.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bf80a72845275afea99e7f2b434723d3bc7e38470fcd1c7ed39a599c73319a53" -dependencies = [ - "getrandom 0.4.3", - "js-sys", - "wasm-bindgen", -] - [[package]] name = "version_check" version = "0.9.5"

Cargo.toml @@ -110,7 +110,6 @@ tokio-postgres = { version = "0.7", default-features = false, features = [ "runtime", ] } -uuid = { version = "1", features = ["v4"] } # These lint configurations were originally pulled from [Evan Schwartz][1]. # [1]: https://emschwartz.me/your-clippy-config-should-be-stricter/

crates/cli/git-ents/Cargo.toml @@ -35,7 +35,6 @@ tempfile = { workspace = true } thiserror = { workspace = true } tokio = { workspace = true } -uuid = { workspace = true } [dev-dependencies] axum = { workspace = true }

crates/forge/ents-forge/Cargo.toml @@ -17,7 +17,6 @@ gix-object = { workspace = true } gix-ref-store = { workspace = true } thiserror = { workspace = true } -uuid = { workspace = true } [dev-dependencies] ents-testutil = { workspace = true }

crates/cli/ents-web/src/render.rs @@ -40,12 +40,13 @@ /// ``` /// use ents_model::{Member, Provenance}; /// -/// let member = Member::new("ssh-ed25519 AAAA... jdc", Provenance::AdminRegistered); +/// let member = Member::new("jdc", "ssh-ed25519 AAAA... jdc", Provenance::AdminRegistered); /// let rows = ents_web::render::fields(&member); -/// assert_eq!(rows[0].0, "key"); -/// assert!(rows[0].1.contains("ssh-ed25519")); -/// assert_eq!(rows[1].0, "state"); -/// assert_eq!(rows[1].1, "Active"); +/// assert_eq!(rows[0].0, "id"); +/// assert_eq!(rows[1].0, "key"); +/// assert!(rows[1].1.contains("ssh-ed25519")); +/// assert_eq!(rows[2].0, "state"); +/// assert_eq!(rows[2].1, "Active"); /// ``` #[must_use] pub fn fields<T: Facet<'static>>(value: &T) -> Vec<FieldRow> { @@ -134,7 +135,7 @@ /// use ents_model::{Member, Provenance}; /// /// let rows = vec![ -/// ("jdc".to_owned(), Ok(Member::new("key-a", Provenance::AdminRegistered))), +/// ("jdc".to_owned(), Ok(Member::new("jdc", "key-a", Provenance::AdminRegistered))), /// ("legacy".to_owned(), Err("object ... is not a blob".to_owned())), /// ]; /// let rendered = ents_web::render::list_table(&rows, "username", |id| format!("/members/{id}")).into_string(); @@ -248,18 +249,22 @@ #[rstest] // @relation(roots.web-agnostic, scope=function, role=Verifies) fn fields_walks_every_declared_field_in_order_for_any_kernel_entity() { - let member = Member::new("ssh-ed25519 AAAA... jdc", Provenance::AdminRegistered); + let member = Member::new( + "jdc", + "ssh-ed25519 AAAA... jdc", + Provenance::AdminRegistered, + ); let rows = fields(&member); assert_eq!( rows.iter().map(|(name, _)| *name).collect::<Vec<_>>(), - vec!["key", "state", "provenance"] + vec!["id", "key", "state", "provenance"] ); } #[rstest] // @relation(roots.web-agnostic, scope=function, role=Verifies) fn an_enum_field_renders_its_variant_name_not_a_placeholder() { - let member = Member::new("key", Provenance::AdminRegistered); + let member = Member::new("jdc", "key", Provenance::AdminRegistered); let rows = fields(&member); let (_, state) = rows .iter() @@ -270,7 +275,7 @@ } #[rstest] - #[case::member(Member::new("k", Provenance::AdminRegistered))] + #[case::member(Member::new("jdc", "k", Provenance::AdminRegistered))] // @relation(roots.web-agnostic, scope=function, role=Verifies) fn the_same_generic_view_renders_every_entity_type(#[case] member: Member) { // Same call, no type-specific branch -- this is the whole point of @@ -280,6 +285,7 @@ assert!(view(&member).into_string().contains("provenance")); assert!( view(&Effect { + name: "unit".to_owned(), trigger: "rev(refs/heads/main)".to_owned(), toolchains: vec![], run: "true".to_owned(), @@ -310,7 +316,7 @@ fn list_table_derives_its_columns_from_the_first_readable_rows_own_shape() { let rows = vec![( "jdc".to_owned(), - Ok(Member::new("key", Provenance::AdminRegistered)), + Ok(Member::new("jdc", "key", Provenance::AdminRegistered)), )]; let markup = list_table(&rows, "username", |id| format!("/members/{id}")).into_string(); assert!(markup.contains("username")); @@ -324,7 +330,7 @@ let rows = vec![ ( "jdc".to_owned(), - Ok(Member::new("key", Provenance::AdminRegistered)), + Ok(Member::new("jdc", "key", Provenance::AdminRegistered)), ), ( "legacy".to_owned(),

crates/cli/ents-web/src/router.rs @@ -52,7 +52,7 @@ .route("/commit/{oid}", get(pages::commits::show::<O>)) .route("/commit/{oid}/review", post(pages::commits::review::<O>)) .route( - "/reviews/{id}/comment", + "/reviews/{target}/{member}/comment", post(pages::commits::review_comment::<O>), ) .route("/files", get(pages::files::root::<O>))

crates/cli/ents-web/tests/router.rs @@ -2211,6 +2211,7 @@ &objects, effect_name, &Effect { + name: "ci".to_owned(), trigger: "rev(refs/heads/main)".to_owned(), toolchains: vec![], run: "true".to_owned(),

crates/cli/git-ents/src/cli.rs @@ -99,9 +99,10 @@ #[facet(args::subcommand)] action: IssueAction, }, - /// Review a commit: a verdict plus a body at `refs/meta/reviews/<id>`, - /// with a retention pin at `refs/meta/pins/reviews/<id>` keeping the - /// reviewed commit reachable. + /// Review a commit: a verdict plus a body at + /// `refs/meta/reviews/<target>/<member>`, with a retention pin at + /// `refs/meta/pins/reviews/<target>/<member>` keeping the reviewed + /// commit reachable. Review { /// The review action to run. #[facet(args::subcommand)]

crates/cli/git-ents/src/exe.rs @@ -223,7 +223,9 @@ let _ = writeln!( out, "{}\t{}\t{}", - row.id, row.comment.state, row.comment.body + ents_forge::abbreviate_id(&row.id), + row.comment.state, + row.comment.body ); } } @@ -287,7 +289,13 @@ match action { IssueAction::List => { for (id, issue) in commands::issue::list(&root)? { - let _ = writeln!(out, "{id}\t{}\t{}", issue.state, issue.title); + let _ = writeln!( + out, + "{}\t{}\t{}", + ents_forge::abbreviate_id(&id), + issue.state, + issue.title + ); } } IssueAction::Show { id } => { @@ -342,24 +350,32 @@ verdict, body, }; - let id = commands::review::new(&root, new, key)?; - let _ = writeln!(out, "reviewed {id}"); + let target = commands::review::new(&root, new, key)?; + let _ = writeln!(out, "reviewed {}", ents_forge::abbreviate_id(&target)); } ReviewAction::List { target } => { - for (id, review) in commands::review::list(&root, target)? { - let _ = writeln!(out, "{id}\t{}\t{}", review.commit(), review.verdict); + for ((review_target, member), review) in commands::review::list(&root, target)? { + let _ = writeln!( + out, + "{}\t{member}\t{}\t{}", + ents_forge::abbreviate_id(&review_target), + review.target(), + review.verdict + ); } } - ReviewAction::Show { id } => { - let (review, thread) = commands::review::show(&root, &id)?; - let _ = writeln!(out, "commit: {}", review.commit()); + ReviewAction::Show { target, member } => { + let (review, thread) = commands::review::show(&root, &target, &member)?; + let _ = writeln!(out, "target: {}", review.target()); let _ = writeln!(out, "verdict: {}", review.verdict); let _ = writeln!(out, "body: {}", review.body); for (comment_id, comment) in thread { let _ = writeln!( out, - "comment {comment_id}\t{}\t{}", - comment.state, comment.body + "comment {}\t{}\t{}", + ents_forge::abbreviate_id(&comment_id), + comment.state, + comment.body ); } }

crates/cli/git-ents/src/lib.rs @@ -63,7 +63,7 @@ //! }; //! let identity = Identity { actor, sign: &|payload| signer.sign(payload) }; //! -//! let member = ents_model::Member::new(signer.public_openssh(), Provenance::AdminRegistered); +//! let member = ents_model::Member::new("jdc", signer.public_openssh(), Provenance::AdminRegistered); //! let name = ents_model::namespace::member_ref(&MemberId::new("jdc")).expect("valid"); //! let outcome = propose_entity( //! &root.refs, &root.objects, &root.events, name.clone(), &member,

crates/cli/git-ents/tests/reconcile.rs @@ -18,7 +18,7 @@ mod common; -use ents_model::Effect; +use ents_model::{Effect, ResultRecord, Status}; use git_ents::root::HostedRoot; use gix_object::{Commit, Kind, Write as _}; use gix_ref_store::{Expected, RefEdit, RefStore}; @@ -65,6 +65,7 @@ fn define_effect(root: &HostedRoot, name: &str, trigger: &str) { let tree = facet_git_tree::serialize_into( &Effect { + name: name.to_owned(), trigger: trigger.to_owned(), toolchains: vec![], run: "true".to_owned(), @@ -158,10 +159,15 @@ let oid = advance_branch(&root, "refs/heads/main", 100); // Record a result directly (bypassing `write_result`'s signing - // requirement — this test only needs the ref to exist). + // requirement) — a full `ResultRecord`, not a bare `Status`, since + // `Evaluator::outstanding` strictly decodes the results tree as one + // (`model.result-identity`). let short = &oid.to_string()[..12]; - let status_tree = facet_git_tree::serialize_into(&ents_model::Status::Pass, &root.objects) - .expect("serialize"); + let status_tree = facet_git_tree::serialize_into( + &ResultRecord::new("unit", oid, Status::Pass), + &root.objects, + ) + .expect("serialize"); let commit = gix_object::Commit { tree: status_tree, parents: Default::default(),

crates/cli/git-ents/tests/review.rs @@ -2,8 +2,10 @@ //! composition root (`roots.local`): reviewing a commit writes both refs //! `model.review` requires — the entity ref and its retention pin //! (`model.review-pin`), the pin's parents including the reviewed commit -//! and its tree the empty tree — and a review's discussion thread -//! surfaces comments naming it as their context (`model.comment-context`). +//! and its tree the empty tree — a review's discussion thread surfaces +//! comments naming it as their context (`model.comment-context`), and +//! re-reviewing a descendant advances the same composite-keyed ref +//! fast-forward rather than minting a new one (`model.review-pin`). #![allow( clippy::expect_used, @@ -18,7 +20,7 @@ use ents_forge::comment::NewComment; use ents_forge::review::NewReview; -use git_ents::commands::{comment, review}; +use git_ents::commands::{comment, members, review}; use git_ents::root::LocalRoot; use gix_object::{CommitRef, Find, Write as _}; use gix_ref_store::RefStoreRead as _; @@ -61,34 +63,38 @@ } /// `model.review`, `model.review-pin`: `git ents review new` writes both -/// the review's own entity ref and its retention pin, and the pin's tip -/// commit is a merge-shaped, empty-tree commit whose parents include the -/// reviewed commit — the reachability edge `model.review-pin` requires. -// @relation(model.review, model.review-pin, roots.local, scope=function, role=Verifies) +/// the review's own entity ref (keyed `reviews/<target>/<member>`) and its +/// retention pin, and the pin's tip commit is a merge-shaped, empty-tree +/// commit whose parents include the reviewed commit — the reachability +/// edge `model.review-pin` requires. +// @relation(model.review, model.review-pin, meta-ref.identity-binding, roots.local, scope=function, role=Verifies) #[test] fn review_new_writes_both_refs_with_the_pin_parented_on_the_reviewed_commit() { let fixture = common::Fixture::new(1); let reviewed = commit_file(fixture.path(), "file.txt", "line one\n"); let root = LocalRoot::open(fixture.path()).expect("opens"); + members::add(&root, "reviewer", None, Some(fixture.key_path.clone())).expect("enrolls"); let new = NewReview { target: "HEAD".to_owned(), verdict: "approve".to_owned(), body: "looks good".to_owned(), }; - let id = review::new(&root, new, Some(fixture.key_path.clone())).expect("reviews"); + let target = review::new(&root, new, Some(fixture.key_path.clone())).expect("reviews"); // The entity ref exists and reads back verdict, body, and the // reviewed commit as a plain data field — no pin read required. - let (found, _thread) = review::show(&root, &id).expect("shows"); + let (found, _thread) = review::show(&root, &target, "reviewer").expect("shows"); assert_eq!(found.verdict, "approve"); assert_eq!(found.body, "looks good"); - assert_eq!(found.commit(), reviewed); + assert_eq!(found.target(), reviewed); // The pin ref exists; its tip's parents include the reviewed commit, // and its tree is the empty tree — the sole exception to // `meta-ref.namespace`'s tree-is-the-entity shape. - let pin_ref = ents_model::namespace::review_pin_ref(&id).expect("valid"); + let pin_ref = + ents_model::namespace::review_pin_ref(&target, &ents_model::MemberId::new("reviewer")) + .expect("valid"); let pin_tip = root .refs .get(pin_ref.as_ref()) @@ -113,21 +119,22 @@ } /// `model.comment-context`, `model.review`: a comment naming -/// `reviews/<id>` as its context surfaces in `git ents review show`'s -/// thread — the review itself stores no list of its comments. +/// `reviews/<target>/<member>` as its context surfaces in `git ents review +/// show`'s thread — the review itself stores no list of its comments. // @relation(model.review, model.comment-context, roots.local, scope=function, role=Verifies) #[test] fn review_show_surfaces_a_context_comment() { let fixture = common::Fixture::new(1); commit_file(fixture.path(), "file.txt", "line one\n"); let root = LocalRoot::open(fixture.path()).expect("opens"); + members::add(&root, "reviewer", None, Some(fixture.key_path.clone())).expect("enrolls"); let new = NewReview { target: "HEAD".to_owned(), verdict: "request-changes".to_owned(), body: "one nit".to_owned(), }; - let id = review::new(&root, new, Some(fixture.key_path.clone())).expect("reviews"); + let target = review::new(&root, new, Some(fixture.key_path.clone())).expect("reviews"); let draft = NewComment { body: "please rename this".to_owned(), @@ -135,44 +142,104 @@ lines: None, rev: "HEAD".to_owned(), worktree: false, - context: Some(format!("reviews/{id}")), + context: Some(format!("reviews/{target}/reviewer")), parent: None, }; comment::add(&root, draft, Some(fixture.key_path.clone())).expect("comments"); - let (_review, thread) = review::show(&root, &id).expect("shows"); + let (_review, thread) = review::show(&root, &target, "reviewer").expect("shows"); assert_eq!(thread.len(), 1); assert_eq!(thread[0].1.body, "please rename this"); } /// `git ents review list [--target rev]`: filtering by target keeps only -/// reviews of that commit. +/// reviews of that commit — exercised across two different reviewers +/// (rather than one reviewer reviewing two commits) since, per +/// `model.review-pin`, one member reviewing a descendant commit advances +/// their existing thread rather than opening a second one; two distinct +/// review entities need two distinct reviewers. // @relation(model.review, roots.local, scope=function, role=Verifies) #[test] fn review_list_filters_by_target() { let fixture = common::Fixture::new(1); + let other_key = fixture.path().join(".id_ed25519_bob"); + common::write_key(&other_key, 2); let first = commit_file(fixture.path(), "file.txt", "line one\n"); let second = commit_file(fixture.path(), "file.txt", "line one\nline two\n"); let root = LocalRoot::open(fixture.path()).expect("opens"); + members::add(&root, "alice", None, Some(fixture.key_path.clone())).expect("enrolls alice"); + members::add(&root, "bob", None, Some(other_key.clone())).expect("enrolls bob"); let review_of_first = NewReview { target: first.to_string(), verdict: "approve".to_owned(), body: String::new(), }; - let first_id = + let first_target = review::new(&root, review_of_first, Some(fixture.key_path.clone())).expect("reviews"); let review_of_second = NewReview { target: second.to_string(), verdict: "approve".to_owned(), body: String::new(), }; - review::new(&root, review_of_second, Some(fixture.key_path.clone())).expect("reviews"); + review::new(&root, review_of_second, Some(other_key)).expect("reviews"); let all = review::list(&root, None).expect("lists"); assert_eq!(all.len(), 2); let filtered = review::list(&root, Some(first.to_string())).expect("lists"); assert_eq!(filtered.len(), 1); - assert_eq!(filtered[0].0, first_id); + assert_eq!(filtered[0].0.0, first_target); + assert_eq!(filtered[0].0.1, ents_model::MemberId::new("alice")); +} + +/// `model.review-pin`: re-reviewing a descendant of a commit this member +/// already reviewed advances the *same* two refs fast-forward — the +/// composite key stays anchored at the original genesis target, and +/// [`ents_forge::review::Review::target`] moves to the newly reviewed +/// commit — rather than minting a second, unrelated review. +// @relation(model.review, model.review-pin, meta-ref.identity-binding, roots.local, scope=function, role=Verifies) +#[test] +fn re_reviewing_a_descendant_advances_the_same_ref_fast_forward() { + let fixture = common::Fixture::new(1); + let first = commit_file(fixture.path(), "file.txt", "line one\n"); + let second = commit_file(fixture.path(), "file.txt", "line one\nline two\n"); + let root = LocalRoot::open(fixture.path()).expect("opens"); + members::add(&root, "reviewer", None, Some(fixture.key_path.clone())).expect("enrolls"); + + let initial = NewReview { + target: first.to_string(), + verdict: "request-changes".to_owned(), + body: "please address this".to_owned(), + }; + let first_target = + review::new(&root, initial, Some(fixture.key_path.clone())).expect("reviews"); + assert_eq!(first_target, first.to_string()); + + // Re-review the descendant: the CLI's own signer/member resolution + // finds the existing "reviewer" review of `first`, an ancestor of + // `second`, and advances it in place. + let follow_up = NewReview { + target: second.to_string(), + verdict: "approve".to_owned(), + body: "looks good now".to_owned(), + }; + let advanced_target = + review::new(&root, follow_up, Some(fixture.key_path.clone())).expect("re-reviews"); + + // The composite key's target segment is unchanged (still genesis-keyed + // at `first`), but the entity's own recorded target has moved to + // `second`, and there is still exactly one review by this reviewer. + assert_eq!(advanced_target, first_target); + let (review, _thread) = review::show(&root, &first_target, "reviewer").expect("shows"); + assert_eq!(review.target(), second); + assert_eq!(review.verdict, "approve"); + assert_eq!(review.body, "looks good now"); + + let all = review::list(&root, None).expect("lists"); + assert_eq!( + all.len(), + 1, + "re-review advances in place, not a second row" + ); }

crates/forge/ents-forge/src/lib.rs @@ -30,11 +30,13 @@ //! [`review::new`] (writes both the entity ref and the retention pin), //! [`review::list`], [`review::show`] (reusing [`comment::thread`] for //! the review's discussion rather than a second aggregation). -//! - `meta-ref.granularity` — one ref per issue/comment/review -//! (`refs/meta/issues/<id>`, `refs/meta/comments/<id>`, -//! `refs/meta/reviews/<id>`); see [`comment::add`] and [`review::new`] -//! for how an id is generated locally rather than derived from the -//! entity itself. +//! - `meta-ref.granularity`, `meta-ref.identity-binding` — one ref per +//! issue/comment (`refs/meta/issues/<id>`, `refs/meta/comments/<id>`, +//! `<id>` the oid of the entity's own genesis commit), and one ref per +//! `(target, reviewer)` composite key for a review +//! (`refs/meta/reviews/<target>/<member>`); see [`comment::add`], +//! [`issue::new`], and [`review::new`] for how each id derives from the +//! signed content itself rather than being minted. //! - `meta-ref.typed-tree` — every entity module's round-trip test. //! - `anchor.definition`, `anchor.projection`, `anchor.working-tree` — //! [`comment::add`], [`comment::show`], and [`comment::list_projected`], @@ -93,6 +95,47 @@ pub use error::{Error, Result}; pub use issue::Issue; +/// The genesis oid a comment or issue ref's name binds to — the final +/// segment of `refs/meta/comments/<id>` or `refs/meta/issues/<id>` +/// (`meta-ref.identity-binding`), read back from the +/// [`gix::refs::FullName`] `ents_receive::propose_genesis` returns rather +/// than tracked separately, since the ref name and the id are the same +/// string by construction. Shared by [`comment::add`], [`comment::reply`], +/// and [`issue::new`] rather than duplicated per module, unlike this +/// crate's accepted small `commit_tree` copies: this one is a single +/// one-liner with no domain logic to diverge per caller. +pub(crate) fn genesis_id(ref_name: &gix::refs::FullName) -> String { + ref_name + .as_bstr() + .to_string() + .rsplit('/') + .next() + .unwrap_or_default() + .to_owned() +} + +/// Abbreviate a genesis-oid entity id (`model.comment`, `model.issue`) to a +/// short prefix for display — the same seven-hex-character length git's own +/// short object id uses (`model.issue`: "porcelain abbreviates ids the way +/// git abbreviates commit oids"). The full id, never this, belongs in a +/// refname or in machine-readable output (`lens.parity`). +/// +/// # Examples +/// +/// ``` +/// use ents_forge::abbreviate_id; +/// +/// assert_eq!( +/// abbreviate_id("0123456789abcdef0123456789abcdef01234567"), +/// "0123456" +/// ); +/// assert_eq!(abbreviate_id("abc"), "abc"); +/// ``` +#[must_use] +pub fn abbreviate_id(id: &str) -> &str { + id.get(..7).unwrap_or(id) +} + #[cfg(test)] mod tests { use facet::Facet as _;

crates/cli/ents-web/src/pages/account.rs @@ -157,7 +157,20 @@ )?)) } -fn resolve_member_by_key<O: Find>(state: &AppState<O>, pubkey: &str) -> Result<MemberId> { +/// Resolve `pubkey` to the enrolled member whose stored key matches it, or +/// [`Error::NotFound`] when none does — shared with +/// `crate::pages::commits::review`, which needs the same "which member is +/// this session" lookup to key a review's composite +/// `refs/meta/reviews/<target>/<member>` ref (`model.review`). +/// +/// # Errors +/// +/// [`Error::NotFound`] if no enrolled member's key matches `pubkey`; +/// otherwise propagates a ref-store or object read failure. +pub(crate) fn resolve_member_by_key<O: Find>( + state: &AppState<O>, + pubkey: &str, +) -> Result<MemberId> { for entry in state.refs.iter_prefix("refs/meta/member/")? { let (name, tip) = entry?; let path = name.as_bstr().to_string();

crates/cli/ents-web/src/pages/comments.rs @@ -87,7 +87,10 @@ html! { ul { @for (id, comment) in &rows { - li { a href=(format!("/comments/{id}")) { (id) } ": " (comment.body) } + li { + a href=(format!("/comments/{id}")) { (ents_forge::abbreviate_id(id)) } + ": " (comment.body) + } } } h2 { "add a comment" } @@ -143,7 +146,7 @@ &super::RepoHeader::from_state(&state), &super::identity_label(&state), super::Tab::Comments, - &id, + ents_forge::abbreviate_id(&id), html! { dl { dt { "state" } dd { (comment.state) }

crates/cli/ents-web/src/pages/commits.rs @@ -307,8 +307,9 @@ /// Every review targeting `commit_id` (`ents_forge::review::list` filtered /// to this commit, `model.review`), each rendering its verdict prominently, /// its body as AsciiDoc, and its reviewer (from the review ref's own tip -/// commit chain, `meta-ref.trailers` -- a review stores no author field), -/// followed by its discussion: the comments naming `reviews/<id>` as their +/// commit chain, `meta-ref.identity-binding` -- a review stores no author +/// field, only its composite `(target, member)` key), followed by its +/// discussion: the comments naming `reviews/<target>/<member>` as their /// context (`ents_forge::comment::thread`, `model.comment-context`), /// rendered through the same shared `super::comments::thread_section` an /// issue's thread uses. A "start a review" form closes the section @@ -330,16 +331,16 @@ let return_to = format!("/commit/{oid}"); html! { h2 { "reviews" } - @for (id, review) in &reviews { + @for ((target, member), review) in &reviews { div.card { div.comment-meta { span.verdict { (review.verdict) } - @let reviewer = ents_model::namespace::review_ref(id) + span.author { (member) } + @let reviewer = ents_model::namespace::review_ref(target, member) .ok() .and_then(|ref_name| state.refs.get(ref_name.as_ref()).ok().flatten()) .and_then(|tip| super::commit_authorship(&*state.objects(), tip).ok()); - @if let Some((author, seconds)) = &reviewer { - span.author { (author) } + @if let Some((_author, seconds)) = &reviewer { span { (super::ago(*seconds)) } } } @@ -349,22 +350,28 @@ @let thread = ents_forge::comment::thread( state.refs.as_ref(), &*state.objects(), - &format!("reviews/{id}"), + &format!("reviews/{target}/{member}"), ).unwrap_or_default(); (super::comments::thread_section(state, session, &thread, &return_to)) - (review_comment_form(session, id, &return_to)) + (review_comment_form(session, target, member, &return_to)) } } (start_review_form(session, oid)) } } -/// The comment-on-this-review form (`POST /reviews/{id}/comment`): a -/// contextual comment naming `reviews/<id>` (`model.comment-context`), so a -/// review's discussion can start from the web and not only the CLI or lens. -fn review_comment_form(session: &Session, id: &str, return_to: &str) -> Markup { +/// The comment-on-this-review form (`POST /reviews/{target}/{member}/comment`): +/// a contextual comment naming `reviews/<target>/<member>` +/// (`model.comment-context`), so a review's discussion can start from the +/// web and not only the CLI or lens. +fn review_comment_form( + session: &Session, + target: &str, + member: &ents_model::MemberId, + return_to: &str, +) -> Markup { html! { - form method="post" action=(format!("/reviews/{id}/comment")) { + form method="post" action=(format!("/reviews/{target}/{member}/comment")) { (super::csrf_input(session)) input type="hidden" name="return_to" value=(return_to); label { "comment on this review" textarea name="body" {} } @@ -373,7 +380,7 @@ } } -/// The form fields `POST /reviews/{id}/comment` accepts. +/// The form fields `POST /reviews/{target}/{member}/comment` accepts. #[derive(Debug, Deserialize)] pub struct ReviewCommentForm { /// The comment's body text. @@ -386,10 +393,10 @@ return_to: String, } -/// `POST /reviews/{id}/comment`: a comment naming `reviews/<id>` as its -/// context (`model.comment-context`) -- an ordinary -/// [`ents_forge::comment::add`], contextual and unanchored, joining the -/// review's discussion thread the moment it lands. +/// `POST /reviews/{target}/{member}/comment`: a comment naming +/// `reviews/<target>/<member>` as its context (`model.comment-context`) -- +/// an ordinary [`ents_forge::comment::add`], contextual and unanchored, +/// joining the review's discussion thread the moment it lands. /// /// # Errors /// @@ -399,7 +406,7 @@ pub async fn review_comment<O>( State(state): State<Arc<AppState<O>>>, axum::Extension(session): axum::Extension<Session>, - Path(id): Path<String>, + Path((target, member)): Path<(String, String)>, Form(form): Form<ReviewCommentForm>, ) -> Result<impl IntoResponse> where @@ -413,7 +420,7 @@ lines: None, rev: "HEAD".to_owned(), worktree: false, - context: Some(format!("reviews/{id}")), + context: Some(format!("reviews/{target}/{member}")), parent: None, }; let (_comment_id, outcome) = ents_forge::comment::add( @@ -483,18 +490,20 @@ O: Find + Write + Send + 'static, { super::require_csrf(&session, &form.csrf)?; + let member = reviewer_member_id(&state); let identity = state.identity.as_ref(); let new = ents_forge::review::NewReview { target: oid.clone(), verdict: form.verdict, body: form.body, }; - let (_id, outcome) = ents_forge::review::new( + let (_target, outcome) = ents_forge::review::new( state.refs.as_ref(), &*state.objects(), state.events.as_ref(), &state.path, new, + &member, &crate::receive_identity!(identity), state.mode, )?; @@ -502,6 +511,36 @@ Ok(Redirect::to(&format!("/commit/{oid}"))) } +/// The acting session's member id -- the composite review key's +/// `<member>` segment -- resolved the same way +/// [`super::account::resolve_member_by_key`] does, falling back to a short +/// hash of the public key when no enrolled member matches: mirrors +/// `git_ents::commands::serve::build_state`'s identical fallback +/// (`roots.web-signing`: an unenrolled local identity may still review, +/// exactly as it may still browse and comment). +fn reviewer_member_id<O: Find>(state: &AppState<O>) -> ents_model::MemberId { + let pubkey = state.identity.public_openssh(); + super::account::resolve_member_by_key(state, &pubkey) + .unwrap_or_else(|_source| ents_model::MemberId::new(short_key_fingerprint(&pubkey))) +} + +/// The first twelve characters of `pubkey`'s key-material token -- mirrors +/// `git_ents::commands::short_fingerprint`'s identical fallback label. +fn short_key_fingerprint(pubkey: &str) -> String { + let hex: String = pubkey + .split_whitespace() + .nth(1) + .unwrap_or(pubkey) + .chars() + .take(12) + .collect(); + if hex.is_empty() { + "member".to_owned() + } else { + hex + } +} + /// Validate `text` as a full, well-formed object id -- hex characters only, /// at the exact length the served repository's hash kind expects (this /// page does not resolve abbreviated prefixes; [`super::commits::list`]'s

crates/cli/git-ents/src/commands/effect.rs @@ -2,7 +2,7 @@ //! (`model.effect-definition`, `effect.local-run`). use ents_effect::run::{run_effect, short_oid}; -use ents_model::{Effect, Status, namespace}; +use ents_model::{Effect, ResultRecord, Status, namespace}; use ents_receive::{Identity, propose_entity}; use gix_ref_store::RefStoreRead; @@ -57,6 +57,7 @@ let signer = signer(root, key)?; let effect = Effect { + name: name.to_owned(), trigger: on, toolchains, run, @@ -105,7 +106,9 @@ None => None, Some(result_tip) => { let tree = super::commit_tree(&root.objects, result_tip)?; - facet_git_tree::deserialize::<Status>(&tree, &root.objects).ok() + facet_git_tree::deserialize::<ResultRecord>(&tree, &root.objects) + .ok() + .map(|record| record.status) } } } @@ -216,8 +219,8 @@ for entry in root.refs.iter_prefix(&prefix)? { let (_, tip) = entry?; let tree = super::commit_tree(&root.objects, tip)?; - if let Ok(status) = facet_git_tree::deserialize::<Status>(&tree, &root.objects) { - out.push((tip, status)); + if let Ok(record) = facet_git_tree::deserialize::<ResultRecord>(&tree, &root.objects) { + out.push((tip, record.status)); } } Ok(out)

crates/cli/git-ents/src/commands/members.rs @@ -45,7 +45,7 @@ ) -> Result<()> { let signer = signer(root, key)?; let pubkey = pubkey.unwrap_or_else(|| signer.public_openssh()); - let member = Member::new(pubkey, Provenance::AdminRegistered); + let member = Member::new(MemberId::new(username), pubkey, Provenance::AdminRegistered); let name = namespace::member_ref(&MemberId::new(username))?; let identity = Identity { actor: actor(&signer),

crates/cli/git-ents/src/commands/review.rs @@ -1,13 +1,16 @@ //! `git ents review`: a thin wrapper around `ents_forge::review`'s //! business logic — this module only resolves the signer/actor identity -//! against [`LocalRoot`] and translates a reached `Outcome` into a -//! CLI-facing [`Result`] (`crate::mutate::outcome_to_result`), exactly as -//! every other mutation command does. Every operation is the library call -//! itself (`lens.parity`); nothing here re-implements one. +//! against [`LocalRoot`] (plus, for [`new`], the reviewer's own member id — +//! the composite key's `<member>` segment, `meta-ref.identity-binding`) and +//! translates a reached `Outcome` into a CLI-facing [`Result`] +//! (`crate::mutate::outcome_to_result`), exactly as every other mutation +//! command does. Every operation is the library call itself +//! (`lens.parity`); nothing here re-implements one. use ents_forge::comment::Comment; use ents_forge::review; use ents_forge::review::{NewReview, Review}; +use ents_model::MemberId; use ents_receive::Identity; use super::{actor, signer}; @@ -15,8 +18,10 @@ use crate::mutate::outcome_to_result; use crate::root::LocalRoot; -/// `git ents review new`: review a commit, writing both its entity ref and -/// its retention pin. +/// `git ents review new`: review a commit as the signer's own member, +/// writing both its entity ref and its retention pin — or, when this +/// member already has a review of an ancestor of the target, advancing +/// that same review fast-forward (`model.review-pin`). /// /// # Errors /// @@ -25,30 +30,55 @@ /// [`crate::mutate::outcome_to_result`] for how a reached refusal renders. pub fn new(root: &LocalRoot, new: NewReview, key: Option<std::path::PathBuf>) -> Result<String> { let signer = signer(root, key)?; + let member = reviewer_member_id(root, &signer)?; let identity = Identity { actor: actor(&signer), sign: &|payload| signer.sign(payload), }; - let (id, outcome) = review::new( + let (target, outcome) = review::new( &root.refs, &root.objects, &root.events, &root.path, new, + &member, &identity, root.mode(), )?; outcome_to_result(outcome, None)?; - Ok(id) + Ok(target) +} + +/// The member id owning the signer's key — the composite review key's +/// `<member>` segment — via the same key-to-member scan +/// [`super::members::find_by_key`] already performs for `git ents members +/// check`. When the signing key enrolls no member (`roots.local`'s +/// advisory gate never requires enrollment before a local mutation lands), +/// falls back to the same fingerprint-derived placeholder [`super::actor`] +/// already uses for its own commit signature: a composite review key still +/// needs *some* member segment, and `gate.owner-mutation` — not this +/// fallback — is what actually keys ownership once a real deployment's +/// mandatory gate is in force. +/// +/// # Errors +/// +/// Propagates a ref-store or object read failure. +fn reviewer_member_id(root: &LocalRoot, signer: &crate::sign::Signer) -> Result<MemberId> { + let pubkey = signer.public_openssh(); + if let Some((username, _state)) = super::members::find_by_key(root, &pubkey)? { + return Ok(MemberId::new(username)); + } + Ok(MemberId::new(super::short_fingerprint(signer))) } /// `git ents review list [--target rev]`: every review recorded in this -/// repository, optionally filtered to those reviewing `target`. +/// repository, keyed by its composite `(target, member)` segments, +/// optionally filtered to those reviewing `target`. /// /// # Errors /// /// Propagates a ref-store, object read, or revision-resolution failure. -pub fn list(root: &LocalRoot, target: Option<String>) -> Result<Vec<(String, Review)>> { +pub fn list(root: &LocalRoot, target: Option<String>) -> Result<Vec<((String, MemberId), Review)>> { Ok(review::list( &root.refs, &root.objects, @@ -57,12 +87,22 @@ )?) } -/// `git ents review show`: `id`'s review, plus its discussion thread. +/// `git ents review show`: `target`/`member`'s review, plus its discussion +/// thread. /// /// # Errors /// /// [`crate::error::Error::Forge`] (wrapping [`ents_forge::Error::NotFound`]) -/// if `id` has no review ref. -pub fn show(root: &LocalRoot, id: &str) -> Result<(Review, Vec<(String, Comment)>)> { - Ok(review::show(&root.refs, &root.objects, id)?) +/// if `target`/`member` has no review ref. +pub fn show( + root: &LocalRoot, + target: &str, + member: &str, +) -> Result<(Review, Vec<(String, Comment)>)> { + Ok(review::show( + &root.refs, + &root.objects, + target, + &MemberId::new(member), + )?) }

crates/forge/ents-forge/src/comment/cli.rs @@ -69,7 +69,7 @@ #[facet(args::named, default)] worktree: bool, /// Canonical ref path below refs/meta/ of the entity this comment - /// belongs to, e.g. `issues/<id>` or `reviews/<id>`. + /// belongs to, e.g. `issues/<id>` or `reviews/<target>/<member>`. #[facet(args::named)] context: Option<String>, /// Id of the comment this one replies to.

crates/forge/ents-forge/src/comment/command.rs @@ -14,7 +14,7 @@ //! lens are three callers of exactly these functions. use ents_anchor::{Anchor, LineRange, Projection, project, project_worktree, snippet}; -use ents_receive::{Identity, Mode, Outcome, propose_entity}; +use ents_receive::{Identity, Mode, Outcome, propose_entity, propose_genesis}; use facet_git_tree::RawTree; use gix_hash::ObjectId; use gix_object::{CommitRef, Find, Kind, Write}; @@ -201,7 +201,8 @@ /// `git ents comment add`: create a comment about something. /// -/// Returns the generated comment id alongside the raw +/// Returns the comment's id — its genesis commit's own oid +/// (`model.comment`, `meta-ref.identity-binding`) — alongside the raw /// [`Outcome`] `receive` reached — callers interpret it themselves (the /// CLI's own `outcome_to_result`, for instance), the same shape /// `ents_effect::run::run_one` returns its own raw `Outcome` in. @@ -215,7 +216,7 @@ /// not form a valid ref path below `refs/meta/`; [`Error::NotFound`] if /// `parent` names no existing comment (`model.comment-thread`); otherwise /// propagates capture, serialization, or `receive` failures. -// @relation(model.comment, model.comment-state, model.comment-context, model.comment-thread, lens.parity, scope=function) +// @relation(model.comment, model.comment-state, model.comment-context, model.comment-thread, meta-ref.identity-binding, lens.parity, scope=function) pub fn add( refs: &dyn RefStore, objects: &(impl Find + Write), @@ -274,21 +275,24 @@ parent: new.parent, }; - // The comment's id is its own genesis tip's short oid, known only once - // the commit is built — `propose_entity` builds it internally, so this - // command derives the ref name from a locally generated id instead - // (`meta-ref.granularity`: one ref per comment). - let id = uuid::Uuid::new_v4().simple().to_string(); - let ref_name = ents_model::namespace::comment_ref(&id)?; let subject = match &new.path { Some(path) => format!("Comment on {path}"), None => "Comment".to_owned(), }; - let outcome = propose_entity( - refs, objects, events, ref_name, &comment, identity, &subject, mode, + // A comment's id is the oid of its own genesis commit — sign-then-name, + // never a locally minted id (`model.comment`, `meta-ref.identity-binding`). + let (ref_name, outcome) = propose_genesis( + refs, + objects, + events, + &comment, + |oid| ents_model::namespace::comment_ref(&oid.to_string()), + identity, + &subject, + mode, )?; - Ok((id, outcome)) + Ok((crate::genesis_id(&ref_name), outcome)) } /// `git ents comment reply`: a comment whose parent is `parent_id` @@ -318,19 +322,17 @@ context: None, parent: Some(parent_id.to_owned()), }; - let id = uuid::Uuid::new_v4().simple().to_string(); - let ref_name = ents_model::namespace::comment_ref(&id)?; - let outcome = propose_entity( + let (ref_name, outcome) = propose_genesis( refs, objects, events, - ref_name, &comment, + |oid| ents_model::namespace::comment_ref(&oid.to_string()), identity, &format!("Reply to comment {parent_id}"), mode, )?; - Ok((id, outcome)) + Ok((crate::genesis_id(&ref_name), outcome)) } /// `git ents comment resolve`: record state `resolved` as an ordinary

crates/forge/ents-forge/src/comment/entity.rs @@ -17,7 +17,7 @@ /// tool ([`Comment::is_about_nothing`], enforced in [`super::add`]), never /// by the gate, which stays content-agnostic. Author and timestamp come /// from the mutation commit chain rather than a stored field -/// (`meta-ref.trailers`) — `Comment` therefore has no author or timestamp +/// (`meta-ref.identity-binding`) — `Comment` therefore has no author or timestamp /// field, and no reviewer/resolver field either: who changed [`Comment::state`], /// and when, is the mutation chain's answer too (`model.comment-state`). /// @@ -67,7 +67,7 @@ /// comment about a context entity or a parent comment only. pub anchor: Option<RawTree>, /// The canonical ref path below `refs/meta/` of the entity this - /// comment belongs to, such as `issues/<id>` or `reviews/<id>` + /// comment belongs to, such as `issues/<id>` or `reviews/<target>/<member>` /// (`model.comment-context`) — an entity's thread is an aggregation /// query over comments naming it, never a list the entity stores. pub context: Option<String>,

crates/forge/ents-forge/src/issue/command.rs @@ -13,7 +13,7 @@ //! `crate::comment::command` never touches a terminal either. use ents_model::MemberId; -use ents_receive::{Identity, Mode, Outcome, propose_entity}; +use ents_receive::{Identity, Mode, Outcome, propose_entity, propose_genesis}; use gix_hash::ObjectId; use gix_object::{CommitRef, Find, Kind}; @@ -75,13 +75,14 @@ pub labels: Vec<String>, } -/// `git ents issue new`: create an issue at a freshly generated -/// `refs/meta/issues/<id>`. +/// `git ents issue new`: create an issue at `refs/meta/issues/<id>`, where +/// `<id>` is the oid of the issue's own genesis commit — sign-then-name, +/// never a locally minted id (`model.issue`, `meta-ref.identity-binding`). /// /// # Errors /// /// Propagates serialization or `receive` failures. -// @relation(model.issue, lens.parity, scope=function) +// @relation(model.issue, meta-ref.identity-binding, lens.parity, scope=function) pub fn new( refs: &dyn gix_ref_store::RefStore, objects: &(impl Find + gix_object::Write), @@ -97,19 +98,18 @@ assignees: new.assignees, labels: new.labels, }; - let id = uuid::Uuid::new_v4().simple().to_string(); - let ref_name = ents_model::namespace::issue_ref(&id)?; - let outcome = propose_entity( + let subject = format!("Open issue: {}", issue.title); + let (ref_name, outcome) = propose_genesis( refs, objects, events, - ref_name, &issue, + |oid| ents_model::namespace::issue_ref(&oid.to_string()), identity, - &format!("Open issue: {}", issue.title), + &subject, mode, )?; - Ok((id, outcome)) + Ok((crate::genesis_id(&ref_name), outcome)) } /// What `git ents issue edit` changes; a field left `None` is left

crates/forge/ents-forge/src/review/cli.rs @@ -15,9 +15,11 @@ #[repr(u8)] pub enum ReviewAction { /// Review a commit: a verdict plus a body, occupying two refs — the - /// review's own entity ref at `refs/meta/reviews/<id>`, and a retention - /// pin at `refs/meta/pins/reviews/<id>` keeping the reviewed commit (and - /// its ancestry) reachable. + /// review's own entity ref at `refs/meta/reviews/<target>/<member>`, and + /// a retention pin at `refs/meta/pins/reviews/<target>/<member>` keeping + /// the reviewed commit (and its ancestry) reachable. Re-reviewing after + /// the target moves advances the same two refs fast-forward rather than + /// minting new ones. New { /// Revision to review. #[facet(args::named, default = "HEAD")] @@ -42,8 +44,11 @@ /// Show one review: its reviewed commit, verdict, body, and discussion /// thread (comments naming it as their context). Show { - /// The review's id. + /// The review's genesis target segment (`refs/meta/reviews/<target>/*`). #[facet(args::positional)] - id: String, + target: String, + /// The reviewer's member id (`refs/meta/reviews/*/<member>`). + #[facet(args::positional)] + member: String, }, }

crates/forge/ents-forge/src/review/command.rs @@ -1,8 +1,9 @@ //! The `review` command's business logic: review a commit (`model.review`), //! writing both the review's own entity ref and its retention pin -//! (`model.review-pin`), list and read reviews back, and surface a -//! review's discussion thread by reusing [`crate::comment::thread`] rather -//! than duplicating context aggregation. +//! (`model.review-pin`), advancing the same composite-keyed ref on +//! re-review rather than minting a new one, list and read reviews back, +//! and surface a review's discussion thread by reusing +//! [`crate::comment::thread`] rather than duplicating context aggregation. //! //! Generalized over the same trait-object/generic seam //! `crate::comment::command` uses (`&dyn RefStore`/`RefStoreRead`, @@ -10,6 +11,7 @@ //! composition root wires the concrete types and calls these functions, //! never the other way around (`lens.parity`). +use ents_model::MemberId; use ents_receive::{Identity, Mode, Outcome, propose_entity_with_pin}; use gix_hash::ObjectId; use gix_object::{CommitRef, Find, Kind, Write}; @@ -48,6 +50,13 @@ /// the repository at `repo_path`. fn resolve_commit(repo_path: &std::path::Path, rev: &str) -> Result<ObjectId> { let repo = gix::open(repo_path)?; + resolve_in(&repo, rev) +} + +/// [`resolve_commit`], against an already-open `repo` — [`new`] already +/// holds one open (to check re-review ancestry), so it resolves its target +/// through this rather than opening the repository a second time. +fn resolve_in(repo: &gix::Repository, rev: &str) -> Result<ObjectId> { let resolve = || Error::InvalidArgument(format!("cannot resolve {rev} to a commit")); let id = repo .rev_parse_single(rev) @@ -60,13 +69,64 @@ Ok(id) } -/// Read the [`Review`] at `id`'s ref tip, or [`Error::NotFound`] when no -/// such ref exists. -fn review_at(refs: &dyn RefStoreRead, objects: &impl Find, id: &str) -> Result<Review> { - let ref_name = ents_model::namespace::review_ref(id)?; +/// Whether `ancestor` is `descendant` itself, or reachable from it by parent +/// edges — checked via the open repository's own merge-base machinery +/// rather than a bespoke walk. A `false` result on a lookup failure (no +/// shared history at all) is the correct answer, not an error to propagate: +/// it just means this is not the review [`new`] should advance. +fn is_ancestor_or_self(repo: &gix::Repository, ancestor: ObjectId, descendant: ObjectId) -> bool { + ancestor == descendant + || repo + .merge_base(ancestor, descendant) + .is_ok_and(|base| base.detach() == ancestor) +} + +/// This member's existing review, if any, whose recorded target +/// ([`Review::target`]) is `reviewed` itself or one of its ancestors — the +/// fast-forward re-review case `model.review-pin` describes: "re-reviewing +/// after the target moves". Returns the found review's own genesis target +/// segment (the refname's `<target>`, parsed back via +/// [`ents_model::namespace::parse_review_ref`]) for [`new`] to advance the +/// same two refs under, rather than minting fresh ones. +fn find_review_to_advance( + refs: &dyn RefStoreRead, + objects: &impl Find, + repo: &gix::Repository, + member: &MemberId, + reviewed: ObjectId, +) -> Result<Option<String>> { + for entry in refs.iter_prefix("refs/meta/reviews/")? { + let (name, tip) = entry?; + let Some((target, entry_member)) = ents_model::namespace::parse_review_ref(name.as_ref()) + else { + continue; + }; + if entry_member != *member { + continue; + } + let tree = commit_tree(objects, tip)?; + let Ok(review) = facet_git_tree::deserialize::<Review>(&tree, objects) else { + continue; + }; + if is_ancestor_or_self(repo, review.target(), reviewed) { + return Ok(Some(target)); + } + } + Ok(None) +} + +/// Read the [`Review`] at `target`/`member`'s ref tip, or [`Error::NotFound`] +/// when no such ref exists. +fn review_at( + refs: &dyn RefStoreRead, + objects: &impl Find, + target: &str, + member: &MemberId, +) -> Result<Review> { + let ref_name = ents_model::namespace::review_ref(target, member)?; let Some(tip) = refs.get(ref_name.as_ref())? else { return Err(Error::NotFound { - what: format!("review {id}"), + what: format!("review {target}/{member}"), }); }; let tree = commit_tree(objects, tip)?; @@ -86,12 +146,21 @@ pub body: String, } -/// `git ents review new`: review `new.target`, writing both refs -/// `model.review` requires — the review's own entity ref at -/// `refs/meta/reviews/<id>`, and the retention pin at -/// `refs/meta/pins/reviews/<id>` keeping the reviewed commit (and its -/// ancestry) reachable (`model.review-pin`) — under one locally generated -/// id shared by both. +/// `git ents review new`: review `new.target` as `member`, writing both +/// refs `model.review` requires — the review's own entity ref at +/// `refs/meta/reviews/<target>/<member>`, and the retention pin at +/// `refs/meta/pins/reviews/<target>/<member>` keeping the reviewed commit +/// (and its ancestry) reachable (`model.review-pin`) — a composite natural +/// key, no minted id anywhere (`meta-ref.identity-binding`). +/// +/// When `member` already has a review whose own recorded target +/// ([`Review::target`]) is `new.target` itself or one of its ancestors, +/// this is a re-review: the *same* two refs advance fast-forward under the +/// original genesis target segment, with [`Review::target`] updated to the +/// newly reviewed commit, rather than a fresh pair being minted +/// (`model.review-pin`: "re-reviewing after the target moves MUST advance +/// the pin fast-forward"). Otherwise this is the review's genesis, keyed by +/// the reviewed commit's own oid. /// /// The two refs travel in one atomic mutation via /// [`propose_entity_with_pin`] (`receive.multi-ref-atomicity`): the @@ -99,68 +168,82 @@ /// transitions together, so a review is never left with its entity written /// but its retention pin missing. One [`Outcome`] covers the whole batch. /// +/// Returns the composite key's target segment (the review's genesis +/// target, unchanged across re-reviews) alongside the reached [`Outcome`]. +/// /// # Errors /// /// [`Error::InvalidArgument`] if `new.target` does not resolve to a commit; /// otherwise propagates serialization or `receive` failures. -// @relation(model.review, model.review-pin, receive.multi-ref-atomicity, lens.parity, scope=function) +// @relation(model.review, model.review-pin, meta-ref.identity-binding, receive.multi-ref-atomicity, lens.parity, scope=function) +#[expect( + clippy::too_many_arguments, + reason = "one field per mutation shape (refs, objects, events, repo, the draft, the acting \ + member, identity, mode), mirroring propose_entity_with_pin's identically-justified \ + shape one layer down" +)] pub fn new( refs: &dyn RefStore, objects: &(impl Find + Write), events: &dyn ents_receive::EventSink, repo_path: &std::path::Path, new: NewReview, + member: &MemberId, identity: &Identity<'_>, mode: Mode, ) -> Result<(String, Outcome)> { - let reviewed = resolve_commit(repo_path, &new.target)?; + let repo = gix::open(repo_path)?; + let reviewed = resolve_in(&repo, &new.target)?; let review = Review::new(reviewed, new.verdict, new.body); - // The review's id is derived locally, once, and shared by both refs - // (`meta-ref.granularity`: one ref per review, one ref per pin) — - // mirroring `crate::comment::command::add`'s own locally generated id. - let id = uuid::Uuid::new_v4().simple().to_string(); + // Re-reviewing after the target moves advances the SAME ref rather than + // minting a new one (`model.review-pin`): find this member's existing + // review, if any, whose own recorded target is an ancestor of (or equal + // to) the commit reviewed now, and advance it in place. + let target_hex = find_review_to_advance(refs, objects, &repo, member, reviewed)? + .unwrap_or_else(|| reviewed.to_string()); let outcome = propose_entity_with_pin( refs, objects, events, - ents_model::namespace::review_ref(&id)?, + ents_model::namespace::review_ref(&target_hex, member)?, &review, - ents_model::namespace::review_pin_ref(&id)?, + ents_model::namespace::review_pin_ref(&target_hex, member)?, reviewed, identity, &format!("Review {reviewed}"), - &format!("Pin review {id}"), + &format!("Pin review {target_hex}/{member}"), mode, )?; - Ok((id, outcome)) + Ok((target_hex, outcome)) } /// `git ents review list [--target rev]`: every review recorded in this -/// repository, optionally filtered to those whose most recently reviewed -/// commit ([`Review::commit`]) resolves to `target`. +/// repository, keyed by its composite `(target, member)` segments +/// (`model.review`), optionally filtered to those whose most recently +/// reviewed commit ([`Review::target`]) resolves to `target`. /// /// # Errors /// /// [`Error::InvalidArgument`] if `target` is given but does not resolve; /// otherwise propagates a ref-store or object read failure. -// @relation(model.review, scope=function) +// @relation(model.review, meta-ref.identity-binding, scope=function) pub fn list( refs: &dyn RefStoreRead, objects: &impl Find, repo_path: &std::path::Path, target: Option<&str>, -) -> Result<Vec<(String, Review)>> { +) -> Result<Vec<((String, MemberId), Review)>> { let target_oid = target .map(|rev| resolve_commit(repo_path, rev)) .transpose()?; let mut out = Vec::new(); for entry in refs.iter_prefix("refs/meta/reviews/")? { let (name, tip) = entry?; - let path = name.as_bstr().to_string(); - let Some(id) = path.strip_prefix("refs/meta/reviews/") else { + let Some((target_hex, member)) = ents_model::namespace::parse_review_ref(name.as_ref()) + else { continue; }; let tree = commit_tree(objects, tip)?; @@ -168,33 +251,34 @@ continue; }; if let Some(target_oid) = target_oid - && review.commit() != target_oid + && review.target() != target_oid { continue; } - out.push((id.to_owned(), review)); + out.push(((target_hex, member), review)); } Ok(out) } -/// `git ents review show`: `id`'s review, plus its discussion thread — -/// every [`Comment`] naming `reviews/<id>` as its context (or a reply into -/// one), reusing [`crate::comment::thread`] rather than a second -/// aggregation query (`model.comment-context`, `model.review`: "the review -/// itself MUST NOT store a list of its comments"). +/// `git ents review show`: `target`/`member`'s review, plus its discussion +/// thread — every [`Comment`] naming `reviews/<target>/<member>` as its +/// context (or a reply into one), reusing [`crate::comment::thread`] rather +/// than a second aggregation query (`model.comment-context`, `model.review`: +/// "the review itself MUST NOT store a list of its comments"). /// /// # Errors /// -/// [`Error::NotFound`] if `id` has no review ref; otherwise propagates a -/// ref-store or object read failure. +/// [`Error::NotFound`] if `target`/`member` has no review ref; otherwise +/// propagates a ref-store or object read failure. // @relation(model.review, model.comment-context, lens.parity, scope=function) pub fn show( refs: &dyn RefStoreRead, objects: &impl Find, - id: &str, + target: &str, + member: &MemberId, ) -> Result<(Review, Vec<(String, Comment)>)> { - let review = review_at(refs, objects, id)?; - let context = format!("reviews/{id}"); + let review = review_at(refs, objects, target, member)?; + let context = format!("reviews/{target}/{member}"); let thread = crate::comment::thread(refs, objects, &context)?; Ok((review, thread)) }

crates/forge/ents-forge/src/review/entity.rs @@ -9,19 +9,23 @@ /// A verdict on a commit, plus a body (`model.review`). /// /// Every review occupies exactly two refs: this entity's own tree at -/// `refs/meta/reviews/<id>`, and a retention pin at -/// `refs/meta/pins/reviews/<id>` anchoring the reviewed content itself -/// (`model.review-pin`) — [`super::new`] writes both. `commit` is the id -/// of the most recently reviewed commit, stored as a plain data field the -/// same way [`ents_anchor::Anchor::commit`] stores its own commit: a -/// `[u8; 20]` field plus a [`Review::commit`] accessor, so reading it back -/// never requires the pin ref — the pin anchors reachability, the entity -/// describes what was reviewed. `approve` and `request-changes` are -/// conventions, not an enum: custom verdicts are schema, not a platform -/// feature (`model.extensibility`), exactly as custom states are for +/// `refs/meta/reviews/<target>/<member>`, and a retention pin at +/// `refs/meta/pins/reviews/<target>/<member>` anchoring the reviewed content +/// itself (`model.review-pin`) — [`super::new`] writes both. `target` is +/// the oid of the most recently reviewed commit, stored as a plain data +/// field the same way [`ents_model::ResultRecord`]'s own `target` field +/// stores the commit it judged: a `[u8; 20]` field plus a [`Review::target`] +/// accessor, so reading it back never requires the pin ref — the pin +/// anchors reachability, the entity describes what was reviewed. At +/// genesis this field equals the refname's `<target>` segment and binds +/// it (`meta-ref.identity-binding`); re-reviewing a descendant advances +/// this field while the refname stays keyed by genesis +/// (`model.review-pin`). `approve` and `request-changes` are conventions, +/// not an enum: custom verdicts are schema, not a platform feature +/// (`model.extensibility`), exactly as custom states are for /// [`crate::Issue`] and [`crate::comment::Comment`]. Reviewer and /// timestamp come from the mutation commit chain rather than a stored -/// field (`meta-ref.trailers`), so `Review` carries no author or +/// field (`meta-ref.identity-binding`), so `Review` carries no author or /// timestamp field — the same omission [`crate::comment::Comment`] makes. /// A review's discussion is [`crate::comment::Comment`] entities naming /// the review as their context (`model.comment-context`); `Review` itself @@ -32,18 +36,18 @@ /// ``` /// use ents_forge::review::Review; /// -/// let commit = gix_hash::ObjectId::from_hex(b"0123456789abcdef0123456789abcdef01234567") +/// let target = gix_hash::ObjectId::from_hex(b"0123456789abcdef0123456789abcdef01234567") /// .expect("valid hex"); -/// let review = Review::new(commit, "approve", "looks good"); +/// let review = Review::new(target, "approve", "looks good"); /// let (root, store) = facet_git_tree::serialize(&review).expect("serialize"); /// let back: Review = facet_git_tree::deserialize(&root, &store).expect("deserialize"); /// assert_eq!(back, review); -/// assert_eq!(back.commit(), commit); +/// assert_eq!(back.target(), target); /// ``` -// @relation(model.review, meta-ref.typed-tree, model.extensibility, scope=file) +// @relation(model.review, meta-ref.identity-binding, meta-ref.typed-tree, model.extensibility, scope=file) #[derive(Debug, Clone, PartialEq, Eq, Facet)] pub struct Review { - pub(crate) commit: [u8; 20], + target: [u8; 20], /// The review's verdict — `approve`, `request-changes`, or any custom /// value a schema defines; not a fixed enum (`model.review`, /// `model.extensibility`). @@ -53,14 +57,14 @@ } impl Review { - /// Build a review of `commit` carrying `verdict` and `body` + /// Build a review of `target` carrying `verdict` and `body` /// (`model.review`). #[must_use] - pub fn new(commit: ObjectId, verdict: impl Into<String>, body: impl Into<String>) -> Self { + pub fn new(target: ObjectId, verdict: impl Into<String>, body: impl Into<String>) -> Self { let mut bytes = [0u8; 20]; - bytes.copy_from_slice(commit.as_slice()); + bytes.copy_from_slice(target.as_slice()); Self { - commit: bytes, + target: bytes, verdict: verdict.into(), body: body.into(), } @@ -68,11 +72,11 @@ /// The id of the most recently reviewed commit (`model.review`): /// reading this never requires the pin ref - /// (`refs/meta/pins/reviews/<id>`) — the pin anchors reachability, the - /// entity describes what was reviewed. + /// (`refs/meta/pins/reviews/<target>/<member>`) — the pin anchors + /// reachability, the entity describes what was reviewed. #[must_use] - pub fn commit(&self) -> ObjectId { - ObjectId::from_bytes_or_panic(&self.commit) + pub fn target(&self) -> ObjectId { + ObjectId::from_bytes_or_panic(&self.target) } } @@ -91,21 +95,21 @@ #[case::custom_verdict("needs-design-doc")] // @relation(model.review, model.extensibility, meta-ref.typed-tree, scope=function, role=Verifies) fn review_round_trips_with_any_verdict_string(#[case] verdict: &str) { - let commit = + let target = ObjectId::from_hex(b"0123456789abcdef0123456789abcdef01234567").expect("valid hex"); - let review = Review::new(commit, verdict, "reviewed the change"); + let review = Review::new(target, verdict, "reviewed the change"); let (root, store) = serialize(&review).expect("serialize"); let back: Review = deserialize(&root, &store).expect("deserialize"); assert_eq!(back, review); - assert_eq!(back.commit(), commit); + assert_eq!(back.target(), target); } #[rstest] // @relation(model.review, scope=function, role=Verifies) - fn commit_accessor_reflects_the_stored_bytes() { - let commit = + fn target_accessor_reflects_the_stored_bytes() { + let target = ObjectId::from_hex(b"fedcba9876543210fedcba9876543210fedcba98").expect("valid hex"); - let review = Review::new(commit, "approve", ""); - assert_eq!(review.commit(), commit); + let review = Review::new(target, "approve", ""); + assert_eq!(review.target(), target); } }