model: make the review verdict a hard enum
commit
0b510cbmodel: make the review verdict a hard enum
Verdict { Approve, RequestChanges, Comment } replaces the open string: a verdict gates decisions, so its vocabulary is platform, not schema, unlike issue and comment states. Parse and render use the kebab-case convention names every surface shows; the web form becomes a closed select; the CLI parses its argument. No review refs existed, so no data migrates.
spec: model.review verdict is a closed vocabulary Assisted-by: Claude:claude-fable-5
Reviews
No reviews of this commit yet — record a verdict below.
Start a review
docs/spec/model.adoc
@@ -196,9 +196,10 @@
while the refname stays keyed by genesis — plus a verdict and a body;
reading the field MUST NOT require the pin ref: the pin anchors, the
entity describes.
-`approve` and `request-changes` are conventions, not an enum, because
-custom verdicts are schema, not platform features
-(<<model.extensibility>>).
+A review's verdict MUST be one of `approve`, `request-changes`, or
+`comment` — a hard enum, unlike issue and comment states
+(<<model.issue>>, <<model.comment-state>>): a verdict gates decisions,
+so its vocabulary is platform, not schema.
A review's discussion MUST be Comment entities naming the review as
their context (<<model.comment-context>>), anchored into the reviewed
code where they concern specific lines (<<anchor.definition>>); the
crates/cli/git-ents/src/exe.rs
@@ -353,7 +353,7 @@
} => {
let new = ents_forge::review::NewReview {
target,
- verdict,
+ verdict: verdict.parse()?,
body,
};
let target = commands::review::new(&root, new, key)?;
crates/cli/git-ents/tests/review.rs
@@ -20,6 +20,7 @@
use ents_forge::comment::NewComment;
use ents_forge::review::NewReview;
+use ents_forge::review::Verdict;
use git_ents::commands::{comment, members, review};
use git_ents::root::LocalRoot;
use gix_object::{CommitRef, Find, Write as _};
@@ -77,7 +78,7 @@
let new = NewReview {
target: "HEAD".to_owned(),
- verdict: "approve".to_owned(),
+ verdict: Verdict::Approve,
body: "looks good".to_owned(),
};
let target = review::new(&root, new, Some(fixture.key_path.clone())).expect("reviews");
@@ -85,7 +86,7 @@
// 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, &target, "reviewer").expect("shows");
- assert_eq!(found.verdict, "approve");
+ assert_eq!(found.verdict, Verdict::Approve);
assert_eq!(found.body, "looks good");
assert_eq!(found.target(), reviewed);
@@ -131,7 +132,7 @@
let new = NewReview {
target: "HEAD".to_owned(),
- verdict: "request-changes".to_owned(),
+ verdict: Verdict::RequestChanges,
body: "one nit".to_owned(),
};
let target = review::new(&root, new, Some(fixture.key_path.clone())).expect("reviews");
@@ -172,14 +173,14 @@
let review_of_first = NewReview {
target: first.to_string(),
- verdict: "approve".to_owned(),
+ verdict: Verdict::Approve,
body: String::new(),
};
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(),
+ verdict: Verdict::Approve,
body: String::new(),
};
review::new(&root, review_of_second, Some(other_key)).expect("reviews");
@@ -209,7 +210,7 @@
let initial = NewReview {
target: first.to_string(),
- verdict: "request-changes".to_owned(),
+ verdict: Verdict::RequestChanges,
body: "please address this".to_owned(),
};
let first_target =
@@ -221,7 +222,7 @@
// `second`, and advances it in place.
let follow_up = NewReview {
target: second.to_string(),
- verdict: "approve".to_owned(),
+ verdict: Verdict::Approve,
body: "looks good now".to_owned(),
};
let advanced_target =
@@ -233,7 +234,7 @@
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.verdict, Verdict::Approve);
assert_eq!(review.body, "looks good now");
let all = review::list(&root, None).expect("lists");
crates/forge/ents-forge/tests/disjointness.rs
@@ -107,7 +107,7 @@
Sample {
name: "Review",
tree: facet_git_tree::serialize_into(
- &Review::new(target, "approve", "looks good"),
+ &Review::new(target, ents_forge::review::Verdict::Approve, "looks good"),
store,
)
.expect("serialize Review"),
crates/cli/ents-web/src/pages/commits.rs
@@ -446,22 +446,21 @@
Ok(Redirect::to(&target))
}
-/// The start-a-review form (`POST /commit/{oid}/review`): a verdict
-/// (`approve`, `request-changes`, or any custom value -- `model.review`
-/// makes these conventions, not an enum, which is exactly why the verdict
-/// field is a free text input with a `datalist` of the conventional
-/// values, never a closed `select`) and a body.
+/// The start-a-review form (`POST /commit/{oid}/review`): a verdict and
+/// a body. The verdict is a closed `select` over
+/// [`ents_forge::review::Verdict`]'s three variants -- `model.review`
+/// makes it a hard enum, unlike issue and comment states.
fn start_review_form(session: &Session, oid: &str) -> Markup {
html! {
form method="post" action=(format!("/commit/{oid}/review")) {
(super::csrf_input(session))
label {
"verdict"
- input type="text" name="verdict" value="approve" list="verdict-values";
- }
- datalist id="verdict-values" {
- option value="approve" {}
- option value="request-changes" {}
+ select name="verdict" {
+ option value="approve" { "approve" }
+ option value="request-changes" { "request-changes" }
+ option value="comment" { "comment" }
+ }
}
label { "body" textarea name="body" {} }
button type="submit" { "Start a Review" }
@@ -508,7 +507,9 @@
let identity = state.identity.as_ref();
let new = ents_forge::review::NewReview {
target: oid.clone(),
- verdict: form.verdict,
+ verdict: form.verdict.parse().map_err(|_unknown| {
+ Error::InvalidArgument(format!("unknown verdict: {}", form.verdict))
+ })?,
body: form.body,
};
let (_target, outcome) = ents_forge::review::new(
crates/forge/ents-forge/src/review/command.rs
@@ -139,9 +139,8 @@
pub struct NewReview {
/// The revision to review; resolved to a commit before writing.
pub target: String,
- /// The review's verdict (`approve`, `request-changes`, or any custom
- /// value — `model.extensibility`).
- pub verdict: String,
+ /// The review's verdict.
+ pub verdict: super::Verdict,
/// The review's body text.
pub body: String,
}
crates/forge/ents-forge/src/review/entity.rs
@@ -6,6 +6,61 @@
use facet::Facet;
use gix_hash::ObjectId;
+/// 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.
+///
+/// Parses from and renders as its kebab-case convention names
+/// (`approve`, `request-changes`, `comment`), the same strings every
+/// surface shows.
+///
+/// # Examples
+///
+/// ```
+/// use ents_forge::review::Verdict;
+///
+/// let verdict: Verdict = "request-changes".parse().expect("known verdict");
+/// assert_eq!(verdict, Verdict::RequestChanges);
+/// assert_eq!(verdict.to_string(), "request-changes");
+/// assert!("needs-design-doc".parse::<Verdict>().is_err());
+/// ```
+// @relation(model.review, scope=type)
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Facet)]
+#[repr(u8)]
+pub enum Verdict {
+ /// The reviewed content is accepted.
+ Approve,
+ /// The reviewed content needs changes before acceptance.
+ RequestChanges,
+ /// Judgment withheld: the review exists for its body and thread.
+ Comment,
+}
+
+impl std::str::FromStr for Verdict {
+ type Err = crate::Error;
+
+ fn from_str(text: &str) -> Result<Self, Self::Err> {
+ match text {
+ "approve" => Ok(Self::Approve),
+ "request-changes" => Ok(Self::RequestChanges),
+ "comment" => Ok(Self::Comment),
+ other => Err(crate::Error::InvalidArgument(format!(
+ "unknown verdict {other:?}: expected approve, request-changes, or comment"
+ ))),
+ }
+ }
+}
+
+impl std::fmt::Display for Verdict {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ f.write_str(match self {
+ Self::Approve => "approve",
+ Self::RequestChanges => "request-changes",
+ Self::Comment => "comment",
+ })
+ }
+}
+
/// A verdict on a commit, plus a body (`model.review`).
///
/// Every review occupies exactly two refs: this entity's own tree at
@@ -20,10 +75,10 @@
/// 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
+/// (`model.review-pin`). The verdict is a hard [`Verdict`] enum — unlike
+/// the open state vocabularies on [`crate::Issue`] and
+/// [`crate::comment::Comment`], a verdict gates decisions, so its
+/// vocabulary is platform, not schema. Reviewer and
/// timestamp come from the mutation commit chain rather than a stored
/// field (`meta-ref.identity-binding`), so `Review` carries no author or
/// timestamp field — the same omission [`crate::comment::Comment`] makes.
@@ -38,7 +93,7 @@
///
/// let target = gix_hash::ObjectId::from_hex(b"0123456789abcdef0123456789abcdef01234567")
/// .expect("valid hex");
-/// let review = Review::new(target, "approve", "looks good");
+/// let review = Review::new(target, ents_forge::review::Verdict::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);
@@ -48,10 +103,9 @@
#[derive(Debug, Clone, PartialEq, Eq, Facet)]
pub struct Review {
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`).
- pub verdict: String,
+ /// The review's verdict (`model.review`): a fixed [`Verdict`], not a
+ /// string.
+ pub verdict: Verdict,
/// The review's body text.
pub body: String,
}
@@ -60,12 +114,12 @@
/// Build a review of `target` carrying `verdict` and `body`
/// (`model.review`).
#[must_use]
- pub fn new(target: ObjectId, verdict: impl Into<String>, body: impl Into<String>) -> Self {
+ pub fn new(target: ObjectId, verdict: Verdict, body: impl Into<String>) -> Self {
let mut bytes = [0u8; 20];
bytes.copy_from_slice(target.as_slice());
Self {
target: bytes,
- verdict: verdict.into(),
+ verdict,
body: body.into(),
}
}
@@ -90,11 +144,11 @@
use super::*;
#[rstest]
- #[case::approve("approve")]
- #[case::request_changes("request-changes")]
- #[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) {
+ #[case::approve(Verdict::Approve)]
+ #[case::request_changes(Verdict::RequestChanges)]
+ #[case::comment(Verdict::Comment)]
+ // @relation(model.review, meta-ref.typed-tree, scope=function, role=Verifies)
+ fn review_round_trips_with_every_verdict(#[case] verdict: Verdict) {
let target =
ObjectId::from_hex(b"0123456789abcdef0123456789abcdef01234567").expect("valid hex");
let review = Review::new(target, verdict, "reviewed the change");
@@ -109,7 +163,7 @@
fn target_accessor_reflects_the_stored_bytes() {
let target =
ObjectId::from_hex(b"fedcba9876543210fedcba9876543210fedcba98").expect("valid hex");
- let review = Review::new(target, "approve", "");
+ let review = Review::new(target, Verdict::Approve, "");
assert_eq!(review.target(), target);
}
}
crates/forge/ents-forge/src/review/mod.rs
@@ -10,4 +10,4 @@
pub use cli::ReviewAction;
pub use command::{NewReview, list, new, show};
-pub use entity::Review;
+pub use entity::{Review, Verdict};