review: withdraw state, hidden from listings
commit
fc942bcreview: withdraw state, hidden from listings
Add a ReviewState { Active, Withdrawn } lifecycle field to the review
entity (#[facet(default)] Active, so every review tree written before
the field existed still decodes). git ents review withdraw retracts a
member’s own review by append-writing the Withdrawn copy — same target,
verdict, and body — onto the same entity+pin refs via the existing
propose_entity_with_pin path; the prior verdict stays in history. It
refuses when the member has no review reaching the target, and relies on
the gate’s existing owner-mutation check on reviews/<target>/<member>
rather than re-checking identity.
The /reviews list and a commit’s reviews section filter Withdrawn rows out of what they render; review::list itself still returns them for the audit trail and the forthcoming review detail page.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Reviews
No reviews of this commit yet — record a verdict below.
Start a review
crates/cli/ents-web/tests/router.rs
@@ -4191,3 +4191,61 @@
let detail = get_body(&router, &format!("/agents/{id}")).await;
assert!(detail.contains("Confirm plan"));
}
+
+/// `GET /reviews` (`crate::pages::reviews`): a withdrawn review stays in
+/// `refs/meta/reviews/*`'s own history (`model.review`, append-only) but
+/// must not render in this aggregate listing, while an ordinary active
+/// review of the same target still does -- the filter this page's own
+/// `list` applies on `ents_forge::review::ReviewState::Withdrawn`.
+#[tokio::test]
+async fn reviews_list_hides_a_withdrawn_review_but_keeps_an_active_one() {
+ let refs = MemRefStore::default();
+ let objects = ObjectStore::default();
+ let target = "0123456789abcdef0123456789abcdef01234567";
+ let reviewed = gix_hash::ObjectId::from_hex(target.as_bytes()).expect("valid hex");
+
+ let active_ref =
+ ents_model::namespace::review_ref(target, &MemberId::new("alice")).expect("valid");
+ write_meta_entity(
+ &refs,
+ &objects,
+ active_ref,
+ &ents_forge::review::Review::new(
+ reviewed,
+ ents_forge::review::Verdict::Approve,
+ "looks good",
+ ),
+ None,
+ 100,
+ );
+
+ let withdrawn_ref =
+ ents_model::namespace::review_ref(target, &MemberId::new("bob")).expect("valid");
+ write_meta_entity(
+ &refs,
+ &objects,
+ withdrawn_ref,
+ &ents_forge::review::Review::new(
+ reviewed,
+ ents_forge::review::Verdict::RequestChanges,
+ "please fix this",
+ )
+ .withdrawn(),
+ None,
+ 100,
+ );
+
+ let state = build_state_with(
+ FixtureIdentity {
+ name: "local-user",
+ key: Keypair::from_seed(1),
+ },
+ refs,
+ objects,
+ );
+ let router = ents_web::router(state);
+
+ let body = get_body(&router, "/reviews").await;
+ assert!(body.contains("alice"), "active review still lists: {body}");
+ assert!(!body.contains("bob"), "withdrawn review must not render: {body}");
+}
crates/cli/git-ents/src/exe.rs
@@ -500,6 +500,10 @@
let target = commands::review::new(&root, new, key)?;
let _ = writeln!(out, "reviewed {}", ents_forge::abbreviate_id(&target));
}
+ ReviewAction::Withdraw { target, key } => {
+ let target = commands::review::withdraw(&root, target, key)?;
+ let _ = writeln!(out, "withdrew {}", ents_forge::abbreviate_id(&target));
+ }
ReviewAction::List { target } => {
for ((review_target, member), review) in commands::review::list(&root, target)? {
let _ = writeln!(
crates/cli/git-ents/tests/review.rs
@@ -20,7 +20,7 @@
use ents_forge::comment::NewComment;
use ents_forge::review::NewReview;
-use ents_forge::review::Verdict;
+use ents_forge::review::{ReviewState, Verdict};
use git_ents::commands::{comment, members, review};
use git_ents::root::LocalRoot;
use gix_object::{CommitRef, Find, Write as _};
@@ -244,3 +244,92 @@
"re-review advances in place, not a second row"
);
}
+
+/// `model.review`: `git ents review withdraw` writes a new `Withdrawn`
+/// entity onto the reviewer's own existing ref, preserving the verdict and
+/// body untouched — append-only, so the prior `Active` commit stays in the
+/// ref's history rather than being replaced by a different shape.
+// @relation(model.review, model.review-pin, roots.local, scope=function, role=Verifies)
+#[test]
+fn withdraw_preserves_verdict_and_body_and_flips_only_state() {
+ 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: Verdict::RequestChanges,
+ body: "please fix this".to_owned(),
+ };
+ let target = review::new(&root, new, Some(fixture.key_path.clone())).expect("reviews");
+
+ let withdrawn_target =
+ review::withdraw(&root, reviewed.to_string(), Some(fixture.key_path.clone()))
+ .expect("withdraws");
+ assert_eq!(withdrawn_target, target, "withdraw advances the same ref");
+
+ let (review, _thread) = review::show(&root, &target, "reviewer").expect("shows");
+ assert_eq!(review.state, ReviewState::Withdrawn);
+ assert_eq!(review.verdict, Verdict::RequestChanges);
+ assert_eq!(review.body, "please fix this");
+ assert_eq!(review.target(), reviewed);
+
+ // The chain is the audit trail: the review still enumerates from
+ // `list` (only the web listings hide a withdrawn row), and there is
+ // still exactly one review row, not a second one.
+ let all = review::list(&root, None).expect("lists");
+ assert_eq!(all.len(), 1);
+ assert_eq!(all[0].1.state, ReviewState::Withdrawn);
+}
+
+/// `model.review`: withdrawing when this member has never reviewed
+/// `target` (or an ancestor of it) is a clear refusal — there is nothing to
+/// withdraw.
+// @relation(model.review, roots.local, scope=function, role=Verifies)
+#[test]
+fn withdraw_refuses_when_no_review_exists() {
+ 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 err = review::withdraw(&root, "HEAD".to_owned(), Some(fixture.key_path.clone()))
+ .expect_err("nothing to withdraw");
+ assert!(
+ matches!(err, git_ents::error::Error::Forge(_)),
+ "expected Error::Forge, got {err:?}"
+ );
+ assert!(
+ err.to_string().contains("not found"),
+ "expected a NotFound refusal, got {err}"
+ );
+}
+
+/// `model.review`: withdrawing an already-withdrawn review is a
+/// no-op-ish re-write, not an error — the same ref simply advances again
+/// with the same `Withdrawn` state.
+// @relation(model.review, roots.local, scope=function, role=Verifies)
+#[test]
+fn withdrawing_an_already_withdrawn_review_is_not_an_error() {
+ 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: Verdict::Approve,
+ body: "looks good".to_owned(),
+ };
+ let target = review::new(&root, new, Some(fixture.key_path.clone())).expect("reviews");
+
+ review::withdraw(&root, "HEAD".to_owned(), Some(fixture.key_path.clone()))
+ .expect("withdraws");
+ review::withdraw(&root, "HEAD".to_owned(), Some(fixture.key_path.clone()))
+ .expect("withdrawing again is not an error");
+
+ let (review, _thread) = review::show(&root, &target, "reviewer").expect("shows");
+ assert_eq!(review.state, ReviewState::Withdrawn);
+ assert_eq!(review.verdict, Verdict::Approve);
+}
crates/cli/ents-web/src/pages/commits.rs
@@ -485,13 +485,16 @@
commit_id: ObjectId,
oid: &str,
) -> Markup {
- let reviews = ents_forge::review::list(
+ let mut reviews = ents_forge::review::list(
state.refs.as_ref(),
&*state.objects(),
&state.path,
Some(&commit_id.to_string()),
)
.unwrap_or_default();
+ // Withdrawn reviews stay in history (`model.review`, append-only) but
+ // drop out of this section, same as `crate::pages::reviews`'s own list.
+ reviews.retain(|(_, review)| review.state != ents_forge::review::ReviewState::Withdrawn);
let return_to = format!("/commit/{oid}");
html! {
h2 { "Reviews" }
crates/cli/ents-web/src/pages/reviews.rs
@@ -34,6 +34,11 @@
{
let mut rows = ents_forge::review::list(state.refs.as_ref(), &*state.objects(), &state.path, None)
.unwrap_or_default();
+ // Withdrawn reviews stay in `refs/meta/reviews/*`'s own history
+ // (append-only, `model.review`) but drop out of this aggregate listing
+ // — a later item's review detail page is where a withdrawn verdict
+ // still surfaces.
+ rows.retain(|(_, review)| review.state != ents_forge::review::ReviewState::Withdrawn);
let repo = gix::open(&state.path).ok();
let mut with_time: Vec<(i64, Markup)> = rows
crates/cli/git-ents/src/commands/review.rs
@@ -50,6 +50,40 @@
Ok(target)
}
+/// `git ents review withdraw`: retract the signer's own review of `target`,
+/// resolving the reviewer's member id exactly as [`new`] does — the
+/// withdrawing member is always the signing identity's own resolved
+/// member, never one named on the command line, so this can never be
+/// pointed at someone else's review (`gate.owner-mutation` refuses it even
+/// if it were).
+///
+/// # Errors
+///
+/// [`crate::error::Error::Forge`] (wrapping [`ents_forge::Error::NotFound`])
+/// if this member has no existing review reaching `target`; otherwise as
+/// [`new`].
+pub fn withdraw(root: &LocalRoot, target: String, 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),
+ author: None,
+ sign: &|payload| signer.sign(payload),
+ };
+ let (target, outcome) = review::withdraw(
+ &root.refs,
+ &root.objects,
+ &root.events,
+ &root.path,
+ &target,
+ &member,
+ &identity,
+ root.mode(),
+ )?;
+ outcome_to_result(outcome, None)?;
+ 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
crates/forge/ents-forge/src/review/cli.rs
@@ -35,6 +35,21 @@
#[facet(args::named)]
key: Option<PathBuf>,
},
+ /// Withdraw this member's own review: writes a new `Withdrawn`-state
+ /// entity onto the *same* two refs the original review occupies,
+ /// preserving its verdict and body — append-only, so the prior verdict
+ /// stays in the ref's history. Refuses when this member has no
+ /// existing review reaching `target`.
+ Withdraw {
+ /// Revision identifying the review to withdraw: resolved exactly
+ /// as `new`'s own target and re-review advance are, so a
+ /// descendant of the reviewed commit still finds it.
+ #[facet(args::named, default = "HEAD")]
+ target: String,
+ /// Key to sign with; defaults to `user.signingkey`.
+ #[facet(args::named)]
+ key: Option<PathBuf>,
+ },
/// List the reviews recorded in this repository.
List {
/// Keep only reviews of this revision.
crates/forge/ents-forge/src/review/command.rs
@@ -259,6 +259,85 @@
Ok(out)
}
+/// `git ents review withdraw`: retract `member`'s own review of `target`,
+/// leaving the prior verdict in history rather than erasing it
+/// (`model.review`). Resolves `target` (a revision) exactly as [`new`]
+/// does, then reuses [`find_review_to_advance`] to locate `member`'s
+/// *existing* review whose recorded target ([`Review::target`]) is
+/// `target` itself or one of its ancestors — the same fast-forward lookup
+/// `new` performs before a re-review, so a withdrawal reaches the review
+/// even if it has since advanced past the commit named here. That review's
+/// [`Review::withdrawn`] copy — same `target`, `verdict`, and `body`, only
+/// `state` flipped — is written back onto the *same* two refs via
+/// [`propose_entity_with_pin`], the identical advance/ref-writing path
+/// `new` uses: no parallel write path exists for withdrawal
+/// (`model.review-pin`, `receive.multi-ref-atomicity`).
+///
+/// Ownership is enforced entirely by `ents-gate`'s existing checks on the
+/// `refs/meta/reviews/<target>/<member>` namespace — `identity_binding`'s
+/// `Namespace::Review` arm (a review must be signed by the exact `member`
+/// its own refname names) and `owner_mutation`'s `Namespace::Review` arm
+/// (only that same signer may advance it) — so this function does not
+/// re-check who `member` is; it only ever builds and writes
+/// `reviews/<target>/<member>`, `member`'s own ref, and lets the gate
+/// refuse anything else the same way it already refuses a mismatched
+/// re-review (`gate.identity-binding`, `gate.owner-mutation`).
+///
+/// Withdrawing an already-withdrawn review is not an error: the found
+/// review's `withdrawn()` copy of a `Withdrawn` review is itself
+/// `Withdrawn`, so this simply re-writes the same state — a harmless
+/// no-op-ish advance, not a special case this function detects.
+///
+/// # Errors
+///
+/// [`Error::InvalidArgument`] if `target` does not resolve to a commit;
+/// [`Error::NotFound`] if `member` has no existing review reaching
+/// `target` — there is nothing to withdraw; otherwise propagates
+/// serialization or `receive` failures.
+// @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, mirroring new's identically-justified shape"
+)]
+pub fn withdraw(
+ refs: &dyn RefStore,
+ objects: &(impl Find + Write),
+ events: &dyn ents_receive::EventSink,
+ repo_path: &std::path::Path,
+ target: &str,
+ member: &MemberId,
+ identity: &Identity<'_>,
+ mode: Mode,
+) -> Result<(String, Outcome)> {
+ let repo = gix::open(repo_path)?;
+ let reviewed = resolve_in(&repo, target)?;
+
+ let target_hex = find_review_to_advance(refs, objects, &repo, member, reviewed)?.ok_or_else(
+ || Error::NotFound {
+ what: format!("review of {reviewed} by {member}"),
+ },
+ )?;
+ let existing = review_at(refs, objects, &target_hex, member)?;
+ let withdrawn = existing.withdrawn();
+ let retained = existing.target();
+
+ let outcome = propose_entity_with_pin(
+ refs,
+ objects,
+ events,
+ ents_model::namespace::review_ref(&target_hex, member)?,
+ &withdrawn,
+ ents_model::namespace::review_pin_ref(&target_hex, member)?,
+ retained,
+ identity,
+ &format!("Withdraw review {retained}"),
+ &format!("Pin review {target_hex}/{member}"),
+ mode,
+ )?;
+
+ Ok((target_hex, outcome))
+}
+
/// `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
crates/forge/ents-forge/src/review/entity.rs
@@ -6,6 +6,71 @@
use facet::Facet;
use gix_hash::ObjectId;
+/// A review's lifecycle state (`model.review`): whether the reviewer's
+/// verdict still stands (`Active`) or the reviewer has retracted it
+/// (`Withdrawn`). Withdrawal is append-only — [`super::command::withdraw`]
+/// writes a new [`Review`] entity carrying this variant onto the *same*
+/// ref chain rather than deleting anything, so a withdrawn verdict remains
+/// in `refs/meta/reviews/<target>/<member>`'s history: the chain is the
+/// audit trail. Web aggregate views (the `/reviews` list, a commit's own
+/// reviews section) filter `Withdrawn` rows out of what they render, but
+/// nothing here or in `super::command` ever removes the ref, the object,
+/// or an earlier commit naming `Active`.
+///
+/// `Active` is this type's [`Default`] and the [`Review`] field carrying it
+/// is `#[facet(default)]` for exactly one reason: every review tree written
+/// before this variant existed has no `state` entry at all, and must still
+/// read back as a plain, unretracted review rather than fail to decode
+/// (backward compatibility with every review recorded before this change).
+///
+/// Parses from and renders as its kebab-case convention names (`active`,
+/// `withdrawn`), the same convention [`Verdict`] follows.
+///
+/// # Examples
+///
+/// ```
+/// use ents_forge::review::ReviewState;
+///
+/// let state: ReviewState = "withdrawn".parse().expect("known state");
+/// assert_eq!(state, ReviewState::Withdrawn);
+/// assert_eq!(state.to_string(), "withdrawn");
+/// assert_eq!(ReviewState::default(), ReviewState::Active);
+/// ```
+// @relation(model.review, meta-ref.typed-tree, scope=type)
+#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Facet)]
+#[repr(u8)]
+pub enum ReviewState {
+ /// The reviewer's verdict still stands.
+ #[default]
+ Active,
+ /// The reviewer has retracted this review; the verdict and body stay
+ /// in history, unread by aggregate views.
+ Withdrawn,
+}
+
+impl std::str::FromStr for ReviewState {
+ type Err = crate::Error;
+
+ fn from_str(text: &str) -> Result<Self, Self::Err> {
+ match text {
+ "active" => Ok(Self::Active),
+ "withdrawn" => Ok(Self::Withdrawn),
+ other => Err(crate::Error::InvalidArgument(format!(
+ "unknown review state {other:?}: expected active or withdrawn"
+ ))),
+ }
+ }
+}
+
+impl std::fmt::Display for ReviewState {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ f.write_str(match self {
+ Self::Active => "active",
+ Self::Withdrawn => "withdrawn",
+ })
+ }
+}
+
/// A review's verdict (`model.review`): a hard enum, unlike issue and
/// comment states — a verdict gates decisions, so its vocabulary is
/// platform, not schema.
@@ -108,11 +173,21 @@
pub verdict: Verdict,
/// The review's body text.
pub body: String,
+ /// Whether this review still stands or has been withdrawn
+ /// (`model.review`). `#[facet(default)]` so a tree written before this
+ /// field existed — no `state` entry at all — deserializes to
+ /// [`ReviewState::Active`] rather than failing to decode; every
+ /// existing `refs/meta/reviews/*` history predates this field and must
+ /// keep reading.
+ #[facet(default)]
+ pub state: ReviewState,
}
impl Review {
/// Build a review of `target` carrying `verdict` and `body`
- /// (`model.review`).
+ /// (`model.review`), initially [`ReviewState::Active`] — every review
+ /// starts active; only [`super::command::withdraw`] ever writes
+ /// [`ReviewState::Withdrawn`].
#[must_use]
pub fn new(target: ObjectId, verdict: Verdict, body: impl Into<String>) -> Self {
let mut bytes = [0u8; 20];
@@ -121,6 +196,7 @@
target: bytes,
verdict,
body: body.into(),
+ state: ReviewState::Active,
}
}
@@ -132,6 +208,21 @@
pub fn target(&self) -> ObjectId {
ObjectId::from_bytes_or_panic(&self.target)
}
+
+ /// A copy of this review with its state advanced to
+ /// [`ReviewState::Withdrawn`], preserving `target`, `verdict`, and
+ /// `body` exactly (`model.review`): [`super::command::withdraw`] writes
+ /// this new entity onto the same ref chain the original review
+ /// occupies — append-only, so the prior [`Active`](ReviewState::Active)
+ /// commit stays reachable in history — rather than mutating anything
+ /// in place.
+ #[must_use]
+ pub fn withdrawn(&self) -> Self {
+ Self {
+ state: ReviewState::Withdrawn,
+ ..self.clone()
+ }
+ }
}
#[cfg(test)]
@@ -166,4 +257,78 @@
let review = Review::new(target, Verdict::Approve, "");
assert_eq!(review.target(), target);
}
+
+ /// The exact shape a `Review` tree had before [`ReviewState`] existed —
+ /// `target`/`verdict`/`body` only, no `state` entry at all. Local to
+ /// this test: it stands in for every `refs/meta/reviews/<target>/*`
+ /// tree already recorded in a real repository before this change
+ /// landed.
+ #[derive(Facet)]
+ struct PreStateReview {
+ target: [u8; 20],
+ verdict: Verdict,
+ body: String,
+ }
+
+ #[rstest]
+ // @relation(model.review, meta-ref.typed-tree, scope=function, role=Verifies)
+ fn a_review_tree_written_before_state_existed_reads_back_as_active() {
+ let target =
+ ObjectId::from_hex(b"0123456789abcdef0123456789abcdef01234567").expect("valid hex");
+ let mut bytes = [0u8; 20];
+ bytes.copy_from_slice(target.as_slice());
+ let legacy = PreStateReview {
+ target: bytes,
+ verdict: Verdict::RequestChanges,
+ body: "reviewed before withdrawal existed".to_owned(),
+ };
+ let (root, store) = serialize(&legacy).expect("serialize the pre-state shape");
+ let back: Review =
+ deserialize(&root, &store).expect("today's Review must still decode a tree with no \
+ state entry");
+ assert_eq!(back.state, ReviewState::Active);
+ assert_eq!(back.verdict, Verdict::RequestChanges);
+ assert_eq!(back.body, "reviewed before withdrawal existed");
+ assert_eq!(back.target(), target);
+ }
+
+ #[rstest]
+ // @relation(model.review, scope=function, role=Verifies)
+ fn withdrawn_preserves_target_verdict_and_body_and_flips_only_state() {
+ let target =
+ ObjectId::from_hex(b"0123456789abcdef0123456789abcdef01234567").expect("valid hex");
+ let review = Review::new(target, Verdict::Approve, "looks good");
+ let withdrawn = review.withdrawn();
+
+ assert_eq!(withdrawn.state, ReviewState::Withdrawn);
+ assert_eq!(withdrawn.verdict, review.verdict);
+ assert_eq!(withdrawn.body, review.body);
+ assert_eq!(withdrawn.target(), review.target());
+
+ // Idempotent-friendly: withdrawing an already-withdrawn review is a
+ // no-op-ish re-write, not an error or a second distinct shape.
+ let withdrawn_again = withdrawn.withdrawn();
+ assert_eq!(withdrawn_again, withdrawn);
+ }
+
+ #[rstest]
+ #[case::active("active", ReviewState::Active)]
+ #[case::withdrawn("withdrawn", ReviewState::Withdrawn)]
+ // @relation(model.review, scope=function, role=Verifies)
+ fn review_state_parses_its_own_display_strings(
+ #[case] text: &str,
+ #[case] expected: ReviewState,
+ ) {
+ let parsed: ReviewState = text.parse().expect("known state");
+ assert_eq!(parsed, expected);
+ assert_eq!(parsed.to_string(), text);
+ }
+
+ #[rstest]
+ // @relation(model.review, scope=function, role=Verifies)
+ fn review_state_rejects_an_unknown_string() {
+ "revoked"
+ .parse::<ReviewState>()
+ .expect_err("not a known review state");
+ }
}
crates/forge/ents-forge/src/review/mod.rs
@@ -9,5 +9,5 @@
mod entity;
pub use cli::ReviewAction;
-pub use command::{NewReview, list, new, show};
-pub use entity::{Review, Verdict};
+pub use command::{NewReview, list, new, show, withdraw};
+pub use entity::{Review, ReviewState, Verdict};