git ents: Merge branch 'worktree-agent-sessions': review + agent-config batch

crates/cli/ents-web/src/router.rs @@ -74,7 +74,15 @@ "/reviews/{target}/{member}/comment", post(pages::commits::review_comment::<O>), ) + .route( + "/reviews/{target}/{member}/withdraw", + post(pages::reviews::withdraw::<O>), + ) .route("/reviews", get(pages::reviews::list::<O>)) + .route( + "/reviews/{target}/{member}", + get(pages::reviews::show::<O>), + ) .route("/files", get(pages::files::root::<O>)) .route("/files/{*path}", get(pages::files::show::<O>)) .route("/meta", get(pages::meta::show::<O>))

crates/cli/ents-web/tests/router.rs @@ -4191,3 +4191,267 @@ 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}"); +} + +/// `GET /reviews/{target}/{member}` (`crate::pages::reviews::show`): a +/// review started through the commit page's own form (`POST +/// /commit/{oid}/review`) gets its own detail page -- the verdict chip, an +/// `active` state badge, the rendered body, and (since the viewing +/// identity is the review's own author) a withdraw control -- and the +/// Reviews split's own sidebar (`reviews_sidebar`) links to it, newest +/// first, beside `GET /reviews`'s own aggregate listing. +#[tokio::test] +// @relation(model.review, lens.parity, scope=function, role=Verifies) +async fn review_detail_page_renders_for_its_own_author_with_a_withdraw_control() { + let dir = seed_repo(&[("src/main.rs", "fn main() {}\n")]); + let oid = head_oid(dir.path()); + let state = build_state_at( + FixtureIdentity { + name: "reviewer", + key: Keypair::from_seed(1), + }, + dir.path().to_owned(), + ); + let router = ents_web::router(state.clone()); + + let (cookie, csrf) = session_cookie_and_csrf(&router, &state, &format!("/commit/{oid}")).await; + let started = router + .clone() + .oneshot( + Request::post(format!("/commit/{oid}/review")) + .header(header::CONTENT_TYPE, "application/x-www-form-urlencoded") + .header(header::COOKIE, cookie.clone()) + .body(Body::from(format!( + "verdict=approve&body=looks+good+to+me&csrf={csrf}" + ))) + .expect("request"), + ) + .await + .expect("in-process call"); + assert!(started.status().is_redirection(), "{:?}", started.status()); + + let commit_page = get_body(&router, &format!("/commit/{oid}")).await; + let review_id = commit_page + .split_once("/reviews/") + .and_then(|(_, rest)| rest.split_once("/comment")) + .map(|(id, _)| id) + .expect("a review comment form links in") + .to_owned(); + + // The sidebar/list both link to the review's own detail page. + let list = get_body(&router, "/reviews").await; + assert!( + list.contains(&format!("href=\"/reviews/{review_id}\"")), + "the sidebar links the active review's own detail page: {list}" + ); + + let detail = get_body(&router, &format!("/reviews/{review_id}")).await; + assert!( + detail.contains("class=\"verdict verdict-approve\""), + "the verdict chip renders: {detail}" + ); + assert!( + detail.contains("looks good to me"), + "the review body renders as its own doc-body: {detail}" + ); + assert!( + detail.contains(">active<"), + "the state badge names the review active: {detail}" + ); + assert!( + detail.contains(&format!("action=\"/reviews/{review_id}/withdraw\"")), + "the review's own author sees a withdraw control: {detail}" + ); + + // Withdrawing redirects back to the same detail page, which still + // renders -- but now with the withdrawn indicator instead of the + // button -- while the sidebar and the aggregate list both drop it. + let (cookie, csrf) = session_cookie_and_csrf(&router, &state, &detail_path(&review_id)).await; + let withdrawn = router + .clone() + .oneshot( + Request::post(format!("/reviews/{review_id}/withdraw")) + .header(header::CONTENT_TYPE, "application/x-www-form-urlencoded") + .header(header::COOKIE, cookie) + .body(Body::from(format!("csrf={csrf}"))) + .expect("request"), + ) + .await + .expect("in-process call"); + assert!( + withdrawn.status().is_redirection(), + "{:?}", + withdrawn.status() + ); + + let detail_after = get_body(&router, &format!("/reviews/{review_id}")).await; + assert!( + detail_after.contains("withdrawn"), + "the withdrawn review's own page states so plainly: {detail_after}" + ); + assert!( + !detail_after.contains(&format!("action=\"/reviews/{review_id}/withdraw\"")), + "a withdrawn review no longer offers the withdraw control: {detail_after}" + ); + + let list_after = get_body(&router, "/reviews").await; + assert!( + !list_after.contains(&format!("href=\"/reviews/{review_id}\"")), + "the withdrawn review drops out of the aggregate list: {list_after}" + ); +} + +/// `format!("/reviews/{{review_id}}")`, spelled once so +/// [`review_detail_page_renders_for_its_own_author_with_a_withdraw_control`] +/// can reuse the same detail path both to fetch a fresh CSRF token and to +/// `POST` the withdraw form against it. +fn detail_path(review_id: &str) -> String { + format!("/reviews/{review_id}") +} + +/// `crate::pages::reviews::show`'s withdraw control only ever renders for +/// the review's *own* author -- a second identity viewing the same +/// still-active review sees the metadata card and the thread, but no +/// withdraw form at all, exactly as `commits::reviews_section` never lets +/// one member's page render a button that would fail +/// `ents_forge::review::withdraw`'s own author check. +#[tokio::test] +async fn review_detail_page_hides_the_withdraw_control_from_a_non_author() { + 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 review_ref = + ents_model::namespace::review_ref(target, &MemberId::new("carol")).expect("valid"); + write_meta_entity( + &refs, + &objects, + review_ref, + &ents_forge::review::Review::new( + reviewed, + ents_forge::review::Verdict::Approve, + "review body from carol", + ), + None, + 100, + ); + + let state = build_state_with( + FixtureIdentity { + name: "onlooker", + key: Keypair::from_seed(2), + }, + refs, + objects, + ); + let router = ents_web::router(state); + + let detail = get_body(&router, &format!("/reviews/{target}/carol")).await; + assert!( + detail.contains("review body from carol"), + "the review still renders for a non-author viewer: {detail}" + ); + assert!( + !detail.contains("/withdraw\""), + "a non-author viewer sees no withdraw control: {detail}" + ); +} + +/// `POST /reviews/{target}/{member}/withdraw` is a state-changing route +/// gated the same way every other mutation in this crate is +/// (`roots.web-session`): no CSRF field at all is rejected outright, +/// regardless of whether the named review even exists. +#[tokio::test] +async fn review_withdraw_is_rejected_without_a_valid_csrf_token() { + let state = build_state(FixtureIdentity { + name: "local-user", + key: Keypair::from_seed(1), + }); + let router = ents_web::router(Arc::clone(&state)); + + let no_csrf = router + .clone() + .oneshot( + Request::post("/reviews/0123456789abcdef0123456789abcdef01234567/carol/withdraw") + .header(header::CONTENT_TYPE, "application/x-www-form-urlencoded") + .body(Body::empty()) + .expect("request"), + ) + .await + .expect("in-process call"); + assert!( + !no_csrf.status().is_success() && !no_csrf.status().is_redirection(), + "a POST with no csrf field must not withdraw a review" + ); + + let (cookie, _csrf) = session_cookie_and_csrf(&router, &state, "/reviews").await; + let wrong = router + .oneshot( + Request::post("/reviews/0123456789abcdef0123456789abcdef01234567/carol/withdraw") + .header(header::CONTENT_TYPE, "application/x-www-form-urlencoded") + .header(header::COOKIE, cookie) + .body(Body::from("csrf=not-the-token")) + .expect("request"), + ) + .await + .expect("in-process call"); + assert_eq!(wrong.status(), StatusCode::BAD_REQUEST); +}

crates/cli/git-ents/src/cli.rs @@ -102,6 +102,17 @@ #[facet(args::subcommand)] action: AccountAction, }, + /// Manage this repository's forge-wide configuration at + /// `refs/meta/config`: today, non-secret agent-runtime defaults (a + /// provider name, a default model id) alongside the gate's own + /// `epoch`/`workers` fields. The API token is never here; it lives in + /// the deployment-time credential seam + /// (`GIT_ENTS_CREDENTIALS_FILE`), never in a signed, replicated tree. + Config { + /// The config action to run. + #[facet(args::subcommand)] + action: ConfigAction, + }, /// Manage the configured effects at `refs/meta/effects/<name>` and run /// them locally. Effect { @@ -317,6 +328,32 @@ }, } +/// `git ents config` actions. +#[derive(Facet)] +#[repr(u8)] +pub enum ConfigAction { + /// Show this repository's current forge-wide configuration. + Show, + /// Narrow the agent-runtime defaults. Each flag is independent: omit + /// one to leave whatever it currently holds untouched rather than + /// clearing it. + Set { + /// The agent runtime's provider name (e.g. `anthropic`); + /// non-secret -- the API token itself is never a flag here, only + /// `git-ents`'s own `GIT_ENTS_CREDENTIALS_FILE` seam. + #[facet(args::named)] + agent_provider: Option<String>, + /// The agent runtime's default model id (e.g. + /// `claude-sonnet-5`), used whenever a session start omits its + /// own `--model`. + #[facet(args::named)] + agent_default_model: Option<String>, + /// Key to sign with; defaults to `user.signingkey`. + #[facet(args::named)] + key: Option<PathBuf>, + }, +} + /// `git ents effect` actions. #[derive(Facet)] #[repr(u8)]

crates/cli/git-ents/src/exe.rs @@ -8,8 +8,8 @@ )] use crate::cli::{ - AccountAction, AgentAction, Cli, CommentAction, EffectAction, HookAction, InboxAction, - IssueAction, MembersAction, RedactAction, ReviewAction, ToolchainAction, Top, + AccountAction, AgentAction, Cli, CommentAction, ConfigAction, EffectAction, HookAction, + InboxAction, IssueAction, MembersAction, RedactAction, ReviewAction, ToolchainAction, Top, }; use crate::commands; use crate::error::Result; @@ -69,6 +69,7 @@ } Top::Members { action } => run_members(action, out), Top::Account { action } => run_account(action, out), + Top::Config { action } => run_config(action, out), Top::Effect { action } => run_effect(action, out), Top::Toolchain { action } => run_toolchain(action, out), Top::Comment { action } => run_comment(action, out), @@ -172,6 +173,34 @@ Ok(()) } +fn run_config(action: ConfigAction, out: &mut impl std::io::Write) -> Result<()> { + let root = LocalRoot::discover(".")?; + match action { + ConfigAction::Show => { + let config = commands::config::show(&root)?; + let _ = writeln!( + out, + "agent_provider: {}", + config.agent_provider.as_deref().unwrap_or("(unset)") + ); + let _ = writeln!( + out, + "agent_default_model: {}", + config.agent_default_model.as_deref().unwrap_or("(unset)") + ); + } + ConfigAction::Set { + agent_provider, + agent_default_model, + key, + } => { + commands::config::set(&root, agent_provider, agent_default_model, key)?; + let _ = writeln!(out, "config updated"); + } + } + Ok(()) +} + fn run_effect(action: EffectAction, out: &mut impl std::io::Write) -> Result<()> { let root = LocalRoot::discover(".")?; match action { @@ -471,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/kernel/ents-gate/src/config.rs @@ -23,11 +23,14 @@ /// /// `model.sdoc` defines no Config entity yet, so this struct is the /// first (and currently only) definition of the config tree's shape; it -/// lives here rather than in `ents-model` because these are the only -/// fields any crate reads today. When configuration grows non-gate fields -/// (description, role rules, ...), the entity moves to `ents-model` and -/// that change is a storage migration like any other struct change -/// (`meta-ref.migration`). +/// lives here rather than in `ents-model` because these were, until now, +/// the only fields any crate reads. `agent_provider`/`agent_default_model` +/// (below) are the first fields this crate itself has no use for — they +/// exist purely for other crates (`ents-web`'s new-session form) to read +/// — but they stay here rather than moving to `ents-model` today: that +/// move is still deferred until configuration grows enough non-gate +/// fields to justify the storage migration (`meta-ref.migration`) in one +/// pass, rather than one field at a time. /// /// `epoch` is `None` on a config written before verification was turned /// on. Once it is `Some`, the gate applies the tip invariant to every @@ -61,6 +64,21 @@ /// narrowing lands. Empty by default: no worker is designated until a /// signed config write adds one. pub workers: Vec<MemberId>, + /// The agent runtime's provider name (e.g. `"anthropic"`), forge-wide; + /// `None` until a signed config write sets one. Non-secret by + /// construction — the credential that authenticates to the provider + /// never lives here or anywhere in `refs/meta/*`, only in the + /// deployment-time seam (`git-ents`'s `credentials.rs`, + /// `GIT_ENTS_CREDENTIALS_FILE`). The agent runtime that would consume + /// this is not built yet; storing and reading it is this field's + /// entire job for now. + pub agent_provider: Option<String>, + /// The agent runtime's default model id (e.g. `"claude-sonnet-5"`), + /// forge-wide; `None` until a signed config write sets one. Read as a + /// fallback ahead of any hardcoded default a caller carries — today, + /// `ents-web`'s new-agent-session form falls back to it before its own + /// compiled-in constant. + pub agent_default_model: Option<String>, } /// The config recorded by the tree of the commit at `oid`, or an @@ -119,3 +137,21 @@ ) -> Result<Vec<MemberId>> { Ok(current_config(refs, objects)?.workers) } + +/// The forge-wide agent provider name currently in force, read from +/// `refs/meta/config`'s tip; `None` when the config ref does not exist or +/// names no provider. Public (unlike [`designated_workers`], which only +/// this crate's gate consults) because the only consumer today, an agent +/// runtime, does not live in this crate. +pub fn agent_provider(refs: &dyn RefStoreRead, objects: &dyn Find) -> Result<Option<String>> { + Ok(current_config(refs, objects)?.agent_provider) +} + +/// The forge-wide default agent model id currently in force, read from +/// `refs/meta/config`'s tip; `None` when the config ref does not exist or +/// names no default — callers fall back to their own compiled-in default +/// (`ents-web`'s new-agent-session form, today) rather than treating +/// `None` as an error. +pub fn agent_default_model(refs: &dyn RefStoreRead, objects: &dyn Find) -> Result<Option<String>> { + Ok(current_config(refs, objects)?.agent_default_model) +}

crates/kernel/ents-gate/src/lib.rs @@ -155,7 +155,7 @@ mod verdict; mod verify; -pub use config::Config; +pub use config::{Config, agent_default_model, agent_provider}; pub use error::{Error, Result}; pub use verdict::{Admission, AdmissionKind, Refusal, Requirement, Verdict}; pub use verify::{Update, verify};

crates/kernel/ents-gate/tests/gate.rs @@ -856,6 +856,7 @@ &Config { epoch: Some(200), workers: vec![MemberId::new(worker_id)], + ..Config::default() }, Some(&f.admin), seconds,

crates/cli/ents-web/src/assets/ents.css @@ -572,6 +572,7 @@ } .picker .opt .dot { width: 7px; height: 7px; border-radius: 50%; background: var(--ink-4); } .picker .opt.active { font-weight: 600; } +.opt-help { font-size: 12px; line-height: 1.5; margin: 8px 0 0; } /* Label picker chips (`labelPicker`): pick existing or type-and-create. */ .label-chip {

crates/cli/ents-web/src/pages/agents.rs @@ -61,6 +61,7 @@ .map(|entry| (entry.refname, entry.error)) .collect(); let default_base = default_base_ref(&state); + let default_model = resolved_default_model(&state); Ok(super::layout_split( &super::RepoHeader::from_state(&state), &super::identity_label(&state), @@ -79,7 +80,7 @@ } div.card { div.card-header { "Start an Agent Session" } - (new_form(&session, &default_base)) + (new_form(&session, &default_base, &default_model)) } } }, @@ -696,18 +697,19 @@ /// The start-a-session form (`POST /agents`, `docs/agent-sessions-plan.adoc`'s /// Phase 3, "mobile-critical"): a prompt textarea, a base-branch text input /// pre-filled with `default_base` ([`default_base_ref`]), a model text -/// input pre-filled with [`DEFAULT_MODEL`], and a closed two-option review -/// policy picker defaulting to `manual` (mirrors -/// `crate::pages::commits::start_review_form`'s identical closed-verdict -/// picker) -- deliberately no toolchain or retry field: the plan's own -/// words are "complexity lives in the session doc, not the form." -fn new_form(session: &Session, default_base: &str) -> Markup { +/// input pre-filled with `default_model` ([`resolved_default_model`]), and +/// a closed two-option review policy picker defaulting to `manual` +/// (mirrors `crate::pages::commits::start_review_form`'s identical +/// closed-verdict picker) -- deliberately no toolchain or retry field: the +/// plan's own words are "complexity lives in the session doc, not the +/// form." +fn new_form(session: &Session, default_base: &str, default_model: &str) -> Markup { html! { form method="post" action="/agents" { (super::csrf_input(session)) label { "prompt" textarea name="prompt" {} } label { "base branch" input type="text" name="base_ref" value=(default_base); } - label { "model" input type="text" name="model" value=(DEFAULT_MODEL); } + label { "model" input type="text" name="model" value=(default_model); } div { p.muted { "review policy" } div.picker { @@ -722,6 +724,12 @@ "auto" } } + p.opt-help.muted { + strong { "Manual" } + " — no review opens on its own; you start one yourself when you want it. " + strong { "Auto" } + " — a review of the result opens automatically once the run finishes." + } } div.composer-buttons { a.composer-cancel href="/agents" { "Cancel" } @@ -731,11 +739,26 @@ } } -/// The model id [`new_form`] pre-fills and [`NewForm::model`] defaults to -/// when a submission omits the field entirely -- the same default id this -/// codebase's own fixtures and `git-ents::agent_worker` tests already use. +/// The model id [`resolved_default_model`] falls back to when +/// `refs/meta/config` names no `agent_default_model` (or does not exist +/// yet) -- the same default id this codebase's own fixtures and +/// `git-ents::agent_worker` tests already use. const DEFAULT_MODEL: &str = "claude-sonnet-5"; +/// The model id [`new_form`] pre-fills and [`NewForm::model`] falls back to +/// when a submission omits the field entirely: `refs/meta/config`'s +/// `agent_default_model` (`ents_gate::agent_default_model`) when a signed +/// config write has set one, else [`DEFAULT_MODEL`] -- a forge-wide +/// default lets an operator change what every new session starts at +/// without touching this crate's own compiled-in fallback, which stays as +/// the floor for a repository that has never configured one. +fn resolved_default_model<O: Find>(state: &AppState<O>) -> String { + ents_gate::agent_default_model(state.refs.as_ref(), &*state.objects()) + .ok() + .flatten() + .unwrap_or_else(|| DEFAULT_MODEL.to_owned()) +} + /// The base ref [`new_form`] pre-fills: `refs/heads/<branch>` for the /// served repository's own current `HEAD` branch /// ([`super::RepoHeader::from_state`]), or plain `HEAD` when that cannot @@ -776,10 +799,11 @@ /// [`default_base_ref`]). #[serde(default = "default_base_ref_field")] base_ref: String, - /// The model id the run executes against; defaults to - /// [`DEFAULT_MODEL`]. - #[serde(default = "default_model_field")] - model: String, + /// The model id the run executes against; `None` when a submission + /// omits the field entirely, resolved by [`resolved_default_model`] + /// (see [`create`]). + #[serde(default)] + model: Option<String>, /// The session's initially resolved review policy: `auto` or `manual`; /// defaults to `manual` (see [`default_review_policy`]). #[serde(default = "default_review_policy")] @@ -788,11 +812,6 @@ csrf: String, } -/// [`NewForm::model`]'s serde default -- see [`DEFAULT_MODEL`]. -fn default_model_field() -> String { - DEFAULT_MODEL.to_owned() -} - /// `POST /agents`: start an agent session owned by the current signing /// identity's resolved member (`ents_forge::agent::new`), signed /// (`roots.web-signing`) on behalf of the current session @@ -818,10 +837,11 @@ super::require_csrf(&session, &form.csrf)?; let member = session_owner(&state); let identity = state.identity.as_ref(); + let model = form.model.unwrap_or_else(|| resolved_default_model(&state)); let new = NewAgentSession { member, prompt: form.prompt, - model: form.model, + model, toolchains: Vec::new(), base_ref: form.base_ref, review_policy: form

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" } @@ -531,8 +534,11 @@ /// 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( +/// web and not only the CLI or lens. Shared with +/// [`super::reviews::show`]'s own review detail page -- the same composer, +/// not a second one, whether the review's thread renders on its home +/// commit's page or on its own. +pub(crate) fn review_comment_form( session: &Session, target: &str, member: &ents_model::MemberId, @@ -768,7 +774,7 @@ O: Find + Write + Send + 'static, { super::require_csrf(&session, &form.csrf)?; - let member = reviewer_member_id(&state); + let member = super::reviewer_member_id(&state); let identity = state.identity.as_ref(); let new = ents_forge::review::NewReview { target: oid.clone(), @@ -791,37 +797,6 @@ 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) - .map(|(id, _member)| id) - .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/ents-web/src/pages/mod.rs @@ -19,12 +19,17 @@ //! and [`agents`] are rail items of their own -- `Tab::Commits`, //! `Tab::Reviews`, `Tab::Issues`, and `Tab::Agents` (Agents, //! `docs/agent-sessions-plan.adoc`'s Phase 3) in [`layout`]'s icon rail, -//! alongside the dashboard, code, threads, and meta items. `reviews::list` -//! (`GET /reviews`) is a read-only aggregate across every commit's own -//! reviews (`commits::reviews_section` renders the same -//! [`ents_forge::review`] entities scoped to one commit; this module has no -//! writes of its own -- every mutation still posts through `commits`'s own -//! routes). [`search`] +//! alongside the dashboard, code, threads, and meta items. [`reviews`]'s +//! `list` (`GET /reviews`) and `show` (`GET /reviews/{target}/{member}`) +//! are a read-only aggregate and a per-review detail page over the same +//! [`ents_forge::review`] entities [`commits::reviews_section`] renders +//! scoped to one commit; starting a review still only ever posts through +//! `commits`'s own route (`POST /commit/{oid}/review`), but withdrawing one +//! (`POST /reviews/{target}/{member}/withdraw`) and commenting on one +//! (`POST /reviews/{target}/{member}/comment`, `commits::review_comment`) +//! are reachable from either page -- a review's own page and its home +//! commit's page render the identical thread and composer, never two. +//! [`search`] //! renders with no rail item active at all; it is reached from the //! `.wb-bar`'s own `.palette` search form rather than any rail item. @@ -651,3 +656,39 @@ }); format!("scope-c{}", hash.checked_rem(6).unwrap_or(0)) } + +/// The acting session's member id -- the composite review key's +/// `<member>` segment -- resolved the same way +/// [`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 or +/// withdraw a review, exactly as it may still browse and comment). Shared +/// by [`super::commits`] (starting a review) and [`super::reviews`] +/// (withdrawing one) -- both need the same "which member is this session, +/// as far as the review namespace is concerned" answer, so it lives here +/// rather than in either page module. +pub(crate) fn reviewer_member_id<O: Find>(state: &AppState<O>) -> ents_model::MemberId { + let pubkey = state.identity.public_openssh(); + account::resolve_member_by_key(state, &pubkey) + .map(|(id, _member)| id) + .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. [`reviewer_member_id`]'s own helper. +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 + } +}

crates/cli/ents-web/src/pages/reviews.rs @@ -1,94 +1,132 @@ -//! `GET /reviews`: every review recorded in this repository, newest -//! first -- a read-only aggregate across commits, alongside -//! `crate::pages::commits`'s own per-commit `reviews_section` rather than -//! replacing it. Every mutation (starting a review, commenting on one) -//! still posts through `commits`'s own routes; this module has none of -//! its own. +//! `GET /reviews`, `GET /reviews/{target}/{member}`, +//! `POST /reviews/{target}/{member}/withdraw`: the review surface's own +//! aggregate list and per-review detail page -- a read-only aggregate +//! across commits, alongside `crate::pages::commits`'s own per-commit +//! `reviews_section` rather than replacing it. Starting a review still only +//! ever posts through `commits`'s own route (`POST /commit/{oid}/review`); +//! commenting on one (`POST /reviews/{target}/{member}/comment`) is +//! `commits::review_comment`, shared verbatim by both pages that render a +//! review's thread. Withdrawing one is this module's own mutation: every +//! read is `ents_forge::review::{list,show}` and the withdraw write is +//! `ents_forge::review::withdraw` -- the web is another caller of that one +//! library func, never a second review-state machine (`lens.parity`). +//! +//! A review's own page ([`show`]) renders even when the review is +//! [`ents_forge::review::ReviewState::Withdrawn`] -- a direct link stays +//! live -- while [`list`] and [`reviews_sidebar`] both filter withdrawn +//! rows out, mirroring `commits::reviews_section`'s own stance: withdrawal +//! is append-only (`model.review`), it retracts a verdict from the +//! aggregate views, never from history or from the one page a direct link +//! still reaches. use std::sync::Arc; +use axum::Form; +use axum::extract::{Path, State}; +use axum::response::{IntoResponse, Redirect}; +use ents_forge::review::{self, Review, ReviewState}; +use ents_model::MemberId; use gix::bstr::ByteSlice as _; use gix_object::{Find, Write}; use maud::{Markup, html}; +use serde::Deserialize; use crate::error::Result; +use crate::session::Session; use crate::state::AppState; -/// `GET /reviews`: list [`ents_forge::review::list`]'s full result (no -/// `target` filter), newest reviewer-commit first. Best effort per row, -/// mirroring `crate::pages::commits::reviews_section`'s own stance: a -/// review whose reviewer-commit chain or target subject fails to read -/// still renders, just without the piece that failed, rather than -/// dropping the row or failing the page. +/// Every non-withdrawn review recorded in this repository +/// (`ents_forge::review::list`, no `target` filter), each paired with the +/// review ref's own tip-commit time when it could be read (`model.review` +/// stores no timestamp field of its own), newest first. The one aggregate +/// read [`list`]'s cards and [`reviews_sidebar`]'s rows both build from, so +/// the withdrawn filter and the ordering are computed in exactly one place. +/// Best effort throughout, mirroring `commits::reviews_section`'s own +/// stance: a review whose reviewer-commit chain fails to read still sorts +/// (last, its time treated as `0`) rather than dropping the row. +fn active_reviews<O: Find + Write>( + state: &AppState<O>, +) -> Vec<(String, MemberId, Review, Option<i64>)> { + let mut rows = review::list(state.refs.as_ref(), &*state.objects(), &state.path, None) + .unwrap_or_default(); + rows.retain(|(_, review)| review.state != ReviewState::Withdrawn); + let mut with_time: Vec<(String, MemberId, Review, Option<i64>)> = rows + .into_iter() + .map(|((target, member), review)| { + let seconds = 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()) + .map(|(_author, seconds)| seconds); + (target, member, review, seconds) + }) + .collect(); + with_time.sort_by_key(|(.., seconds)| std::cmp::Reverse(seconds.unwrap_or(0))); + with_time +} + +/// `GET /reviews`: [`active_reviews`]'s full result rendered as one card +/// per review -- verdict, reviewer, and the target commit's own subject -- +/// beside [`reviews_sidebar`]'s compact newest-first nav (`crate::pages::layout_split`). /// /// # Errors /// /// Propagates a ref-store or object read failure enumerating the reviews /// themselves ([`ents_forge::review::list`]); a per-row read failure /// degrades that row instead of failing the page. -pub async fn list<O>( - axum::extract::State(state): axum::extract::State<Arc<AppState<O>>>, -) -> Result<Markup> +pub async fn list<O>(State(state): State<Arc<AppState<O>>>) -> Result<Markup> where O: Find + Write + Send + 'static, { - let mut rows = ents_forge::review::list(state.refs.as_ref(), &*state.objects(), &state.path, None) - .unwrap_or_default(); + let rows = active_reviews(&state); let repo = gix::open(&state.path).ok(); - let mut with_time: Vec<(i64, Markup)> = rows - .drain(..) - .map(|((target, member), review)| { - 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()); - let seconds = reviewer.as_ref().map_or(0, |(_author, seconds)| *seconds); + let cards: Vec<Markup> = rows + .iter() + .map(|(target, member, review, seconds)| { let subject = repo.as_ref().and_then(|repo| { let oid = gix_hash::ObjectId::from_hex(target.as_bytes()).ok()?; let commit = repo.find_commit(oid).ok()?; let message = commit.message().ok()?; Some(message.title.to_str_lossy().into_owned()) }); - let short = target.get(..7).unwrap_or(&target).to_owned(); - ( - seconds, - html! { - div.card { - div.comment-meta { - span class={ "verdict verdict-" (review.verdict) } { (review.verdict) } - (super::avatar(member.as_str())) - span.author { (member) } - span.spacer {} - a href={ "/commit/" (target) } { code { (short) } } - @if let Some(subject) = &subject { - span.muted { (subject) } - } - } - @if let Some((_author, seconds)) = &reviewer { - span.entry-size { (super::ago(*seconds)) } + let short = target.get(..7).unwrap_or(target).to_owned(); + html! { + div.card { + div.comment-meta { + span class={ "verdict verdict-" (review.verdict) } { (review.verdict) } + (super::avatar(member.as_str())) + span.author { (member) } + span.spacer {} + a href={ "/reviews/" (target) "/" (member) } { code { (short) } } + @if let Some(subject) = &subject { + span.muted { (subject) } } } - }, - ) + @if let Some(seconds) = seconds { + span.entry-size { (super::ago(*seconds)) } + } + } + } }) .collect(); - with_time.sort_by(|(a, _), (b, _)| b.cmp(a)); - Ok(super::layout( + Ok(super::layout_split( &super::RepoHeader::from_state(&state), &super::identity_label(&state), super::Tab::Reviews, "Reviews", + false, + reviews_sidebar(&rows, None), html! { div.readable { - @if with_time.is_empty() { + @if cards.is_empty() { (super::blankslate( "No reviews yet", html! { "Record one from a commit's own page." }, )) } @else { - @for (_seconds, card) in &with_time { + @for card in &cards { (card) } } @@ -96,3 +134,217 @@ }, )) } + +/// The Reviews split's `.tree` sidebar (mirrors `issues::issues_sidebar`): +/// every [`active_reviews`] row as a two-line `.side-row` -- its verdict and +/// reviewer on the title line, the target commit's abbreviated id on the +/// locator line -- linking to [`show`]'s own page, `.active` naming the +/// viewed `(target, member)` pair. Withdrawn reviews are already filtered +/// out of `rows` by [`active_reviews`]; they stay reachable only by a +/// direct link to [`show`], never from this nav. +fn reviews_sidebar( + rows: &[(String, MemberId, Review, Option<i64>)], + active: Option<(&str, &str)>, +) -> Markup { + html! { + div.tree-head { + span { "Reviews" } + } + @if rows.is_empty() { + span.tree-note { "No reviews yet." } + } + @for (target, member, review, _seconds) in rows { + a.side-row.active[active == Some((target.as_str(), member.as_str()))] + href={ "/reviews/" (target) "/" (member) } + { + span.side-title { + span class={ "verdict verdict-" (review.verdict) } { (review.verdict) } + " " (member.as_str()) + } + span.side-meta { + span.locator { "on " (ents_forge::abbreviate_id(target)) } + } + } + } + } +} + +/// `GET /reviews/{target}/{member}`: one review +/// (`ents_forge::review::show`), its verdict/state/target/reviewer metadata +/// card, its body rendered as AsciiDoc, its discussion thread, a comment +/// composer, and -- only for the review's own author while it is still +/// [`ReviewState::Active`] -- a withdraw control. Renders even for a +/// withdrawn review (this module's own doc: a direct link stays live; only +/// [`list`]/[`reviews_sidebar`] hide a withdrawn row). +/// +/// # Errors +/// +/// [`crate::Error::Forge`] (wrapping [`ents_forge::Error::NotFound`]) if +/// `target`/`member` has no review ref at all; otherwise propagates a +/// ref-store or object read failure. +// @relation(model.review, model.comment-context, lens.parity, scope=function) +pub async fn show<O>( + State(state): State<Arc<AppState<O>>>, + axum::Extension(session): axum::Extension<Session>, + Path((target, member)): Path<(String, String)>, +) -> Result<Markup> +where + O: Find + Write + Send + 'static, +{ + let member = MemberId::new(member); + let (review, thread) = + review::show(state.refs.as_ref(), &*state.objects(), &target, &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()); + let body = + crate::asciidoc::to_html(&review.body).unwrap_or_else(|_| html! { p { (review.body) } }); + let return_to = format!("/reviews/{target}/{member}"); + let is_author = super::reviewer_member_id(&state) == member; + // Best-effort: the sidebar listing every other review beside this one + // is navigation chrome, never a reason to fail this review's own page. + let rows = active_reviews(&state); + + Ok(super::layout_split( + &super::RepoHeader::from_state(&state), + &super::identity_label(&state), + super::Tab::Reviews, + &format!("Review of {}", ents_forge::abbreviate_id(&target)), + false, + reviews_sidebar(&rows, Some((&target, member.as_str()))), + html! { + (super::child_crumbs("reviews", "/reviews", ents_forge::abbreviate_id(&target))) + div.readable { + div.card { + dl.entity-view { + dt { "verdict" } + dd { span class={ "verdict verdict-" (review.verdict) } { (review.verdict) } } + dt { "state" } + dd { (state_badge(review.state)) } + dt { "target" } + dd { a href={ "/commit/" (target) } { code { (target) } } } + dt { "reviewer" } + dd { (super::avatar(member.as_str())) " @" (member.as_str()) } + dt { "reviewed" } + dd { + @if let Some((_author, seconds)) = &reviewer { + (super::ago(*seconds)) + } @else { + span.muted { "unknown" } + } + } + } + div.doc-body { (body) } + } + @if review.state == ReviewState::Active { + @if is_author { + (withdraw_form(&session, &target, &member)) + } + } @else { + p.muted { "This review has been withdrawn." } + } + h2 { "Discussion" } + @if thread.is_empty() { + (super::blankslate( + "No comments yet", + html! { "Start the discussion below." }, + )) + } @else { + (crate::pages::comments::thread_section(&state, &session, &thread, &return_to)) + } + div.card { + div.card-header { "Add a comment" } + (super::commits::review_comment_form(&session, &target, &member, &return_to)) + } + } + }, + )) +} + +/// The review detail card's `state` `dd` (see [`show`]): a plain neutral +/// `.chip.chip-pill` naming `active`, or the same grey `.state-closed` +/// treatment `issues::state_chip` gives a closed issue naming `withdrawn` +/// instead -- so a direct link to a retracted verdict states plainly, at a +/// glance, that it no longer stands. +fn state_badge(state: ReviewState) -> Markup { + match state { + ReviewState::Active => html! { + span.chip.chip-pill { "active" } + }, + ReviewState::Withdrawn => html! { + span.chip.chip-pill.state-closed { "withdrawn" } + }, + } +} + +/// The withdraw-this-review control (`POST /reviews/{target}/{member}/withdraw`), +/// rendered by [`show`] only for the review's own author while it is still +/// [`ReviewState::Active`] -- retracting a verdict stays a decision only +/// its author can make, the same way `ents-gate`'s own `owner_mutation` +/// check refuses anyone else's attempt at the ref level +/// (`ents_forge::review::withdraw`'s own doc). +fn withdraw_form(session: &Session, target: &str, member: &MemberId) -> Markup { + html! { + form method="post" action=(format!("/reviews/{target}/{member}/withdraw")) { + (super::csrf_input(session)) + button type="submit" { "Withdraw review" } + } + } +} + +/// The form fields `POST /reviews/{target}/{member}/withdraw` accepts. +#[derive(Debug, Deserialize)] +pub struct WithdrawForm { + /// The per-session CSRF token (`roots.web-session`). + csrf: String, +} + +/// `POST /reviews/{target}/{member}/withdraw`: retract the signed-in +/// member's own review of `target` (`ents_forge::review::withdraw`) -- the +/// web is another caller of that one library func, driving the identical +/// mutation `git ents review withdraw` does. The path's own `member` +/// segment names whose review [`show`] rendered, but the write always +/// targets the *signed-in* identity's own member id +/// ([`super::reviewer_member_id`]), never the path's: this handler can only +/// ever build and write `reviews/<target>/<the signer>`. A member who is +/// not the review's author therefore has no matching +/// `refs/meta/reviews/<target>/<member>` of their own to advance, and the +/// mutation fails with [`ents_forge::Error::NotFound`] rather than +/// touching anyone else's ref -- no divergent ownership check is added +/// here; `ents-gate`'s own `identity_binding`/`owner_mutation` checks on +/// this namespace back the same refusal up independently (see +/// [`ents_forge::review::withdraw`]'s own doc). +/// +/// # Errors +/// +/// [`crate::Error::BadCsrf`] if `form.csrf` does not match; otherwise +/// propagates [`ents_forge::review::withdraw`]'s own failures (including +/// [`ents_forge::Error::NotFound`] when the signed-in member has no review +/// reaching `target`). +// @relation(model.review, roots.web-signing, roots.web-session, lens.parity, scope=function) +pub async fn withdraw<O>( + State(state): State<Arc<AppState<O>>>, + axum::Extension(session): axum::Extension<Session>, + Path((target, _member)): Path<(String, String)>, + Form(form): Form<WithdrawForm>, +) -> Result<impl IntoResponse> +where + O: Find + Write + Send + 'static, +{ + super::require_csrf(&session, &form.csrf)?; + let member = super::reviewer_member_id(&state); + let identity = state.identity.as_ref(); + let (target_hex, outcome) = review::withdraw( + state.refs.as_ref(), + &*state.objects(), + state.events.as_ref(), + &state.path, + &target, + &member, + &crate::receive_identity!(identity, crate::pages::member_author(&session)), + state.mode, + )?; + crate::error::outcome_to_result(outcome)?; + Ok(Redirect::to(&format!("/reviews/{target_hex}/{member}"))) +}

crates/cli/git-ents/src/commands/mod.rs @@ -12,6 +12,7 @@ pub mod agent; pub mod bootstrap; pub mod comment; +pub mod config; pub mod effect; pub mod inbox; pub mod issue;

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/agent/cli.rs @@ -33,7 +33,9 @@ /// The ref the run executes against as its starting point. #[facet(args::named, default = "HEAD")] base: String, - /// The session's initially resolved review policy: auto or manual. + /// The session's initially resolved review policy. `manual` (default): + /// no review opens on its own; you start one yourself. `auto`: a review + /// of the result opens automatically once the run finishes. #[facet(args::named, default = "manual")] review_policy: String, /// The genesis oid of a prior session this one retries.

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};

crates/cli/git-ents/src/commands/config.rs @@ -1,0 +1,96 @@ +//! `git ents config`: forge-wide, non-secret agent-runtime defaults +//! (provider name, default model) recorded in `refs/meta/config` alongside +//! the gate's own `epoch`/`workers` fields (`ents_gate::Config`). +//! +//! The API token is deliberately not here and never will be: it lives only +//! in the deployment-time credential seam (`crate::credentials`, +//! `GIT_ENTS_CREDENTIALS_FILE`), never in a signed, replicated, +//! multi-reader tree. + +use ents_gate::Config; +use ents_model::namespace; +use ents_receive::{Identity, propose_entity}; +use gix_ref_store::RefStoreRead; + +use super::{actor, signer}; +use crate::error::Result; +use crate::mutate::outcome_to_result; +use crate::root::LocalRoot; + +/// `git ents config show`: this repository's current forge-wide +/// configuration -- [`Config::default`] (every field unset) when +/// `refs/meta/config` has no tip yet, the same "absent means unconfigured" +/// reading every `ents_gate::config` reader already gives. +/// +/// # Errors +/// +/// Propagates a ref-store or object read failure, or an unreadable config +/// tree. +pub fn show(root: &LocalRoot) -> Result<Config> { + #[expect( + clippy::expect_used, + clippy::unwrap_in_result, + reason = "CONFIG_REF is a fixed, compile-time-known-valid refname literal" + )] + let name: gix::refs::FullName = namespace::CONFIG_REF + .try_into() + .expect("fixed, valid refname"); + let Some(tip) = root.refs.get(name.as_ref())? else { + return Ok(Config::default()); + }; + let tree = super::commit_tree(&root.objects, tip)?; + Ok(facet_git_tree::deserialize::<Config>(&tree, &root.objects)?) +} + +/// `git ents config set`: narrow the agent-runtime defaults. Each argument +/// is an independent optional narrowing -- omit one to leave whatever it +/// currently holds untouched, rather than resetting it to `None`. Reads +/// the current config first and writes the merged whole back, so a `set` +/// naming only `agent_provider` cannot clobber an `agent_default_model` +/// set earlier, or the gate's own `epoch`/`workers`. +/// +/// # Errors +/// +/// Propagates a ref-store/object read failure or a signing failure; +/// otherwise see [`crate::mutate::outcome_to_result`] for how a reached +/// refusal renders. +pub fn set( + root: &LocalRoot, + agent_provider: Option<String>, + agent_default_model: Option<String>, + key: Option<std::path::PathBuf>, +) -> Result<()> { + let signer = signer(root, key)?; + let mut config = show(root)?; + if let Some(provider) = agent_provider { + config.agent_provider = Some(provider); + } + if let Some(model) = agent_default_model { + config.agent_default_model = Some(model); + } + #[expect( + clippy::expect_used, + clippy::unwrap_in_result, + reason = "CONFIG_REF is a fixed, compile-time-known-valid refname literal" + )] + let name: gix::refs::FullName = namespace::CONFIG_REF + .try_into() + .expect("fixed, valid refname"); + let identity = Identity { + actor: actor(&signer), + author: None, + sign: &|payload| signer.sign(payload), + }; + let outcome = propose_entity( + &root.refs, + &root.objects, + &root.events, + name, + &config, + &identity, + "Set agent config", + root.mode(), + )?; + outcome_to_result(outcome, None)?; + Ok(()) +}

crates/cli/git-ents/tests/config.rs @@ -1,0 +1,65 @@ +//! Integration coverage for `git ents config` against a real local +//! composition root (`roots.local`): narrowing the agent-runtime defaults +//! at `refs/meta/config` without disturbing fields `set` was not told to +//! touch, and reading an unconfigured repository back as +//! `ents_gate::Config::default()`. + +#![allow(clippy::expect_used, reason = "integration test")] + +mod common; + +use ents_gate::Config; +use git_ents::commands::config; +use git_ents::root::LocalRoot; + +/// A repository that has never had `refs/meta/config` written reads back +/// as every field unset -- the same "absent means unconfigured" reading +/// `ents_gate::config`'s own readers give, and what lets an old config +/// (predating `agent_provider`/`agent_default_model`) keep parsing. +// @relation(roots.local, scope=function, role=Verifies) +#[test] +fn absent_config_reads_as_default() { + let fixture = common::Fixture::new(1); + let root = LocalRoot::open(fixture.path()).expect("opens"); + + let config = config::show(&root).expect("shows"); + assert_eq!(config, Config::default()); +} + +/// `set` narrows one field at a time: a later call naming only +/// `agent_default_model` must not clobber an `agent_provider` an earlier +/// call set, and neither call ever touches `workers`/`epoch` (unset by +/// either call). +// @relation(roots.local, scope=function, role=Verifies) +#[test] +fn set_narrows_without_clobbering_other_fields() { + let fixture = common::Fixture::new(1); + let root = LocalRoot::open(fixture.path()).expect("opens"); + + config::set( + &root, + Some("anthropic".to_owned()), + None, + Some(fixture.key_path.clone()), + ) + .expect("sets provider"); + let config = config::show(&root).expect("shows"); + assert_eq!(config.agent_provider.as_deref(), Some("anthropic")); + assert_eq!(config.agent_default_model, None); + assert!(config.workers.is_empty()); + assert_eq!(config.epoch, None); + + config::set( + &root, + None, + Some("claude-sonnet-5".to_owned()), + Some(fixture.key_path.clone()), + ) + .expect("sets default model"); + let config = config::show(&root).expect("shows"); + assert_eq!(config.agent_provider.as_deref(), Some("anthropic")); + assert_eq!( + config.agent_default_model.as_deref(), + Some("claude-sonnet-5") + ); +}

Merge branch 'worktree-agent-sessions': review + agent-config batch

Reviews

Start a review