git ents: effect, forge, cli, web: the auto-review follow-on effect (phase 5)

crates/cli/ents-web/src/router.rs @@ -110,6 +110,7 @@ ) .route("/agents/{id}", get(pages::agents::show::<O>)) .route("/agents/{id}/confirm", post(pages::agents::confirm::<O>)) + .route("/agents/{id}/review", post(pages::agents::open_review::<O>)) .route( "/agents/{id}/chat", get(pages::agent_chat::show::<O>).post(pages::agent_chat::send::<O>),

crates/cli/ents-web/tests/router.rs @@ -3765,6 +3765,177 @@ assert!(!after.contains("not yet recorded")); } +// --------------------------------------------------------------------- +// `crate::pages::agents`'s manual one-tap "Open review" +// (`docs/agent-sessions-plan.adoc`'s Phase 5). +// --------------------------------------------------------------------- + +/// Push a real `refs/heads/<branch>` tip directly through `state`'s own ref +/// store -- a branch ref needs no signature at all +/// (`gate.principled-split`), mirroring `git-ents`'s own +/// `tests/agent.rs::advance_branch` but against this crate's `AppState` +/// rather than a `LocalRoot`. +fn advance_branch(state: &AppState<ObjectStore>, branch: &str, seconds: i64) -> gix_hash::ObjectId { + let tree = state.objects().write(&Tree::empty()).expect("tree"); + let oid = write_commit( + &*state.objects(), + &CommitSpec { + tree, + parents: vec![], + message: format!("agent-exec output for {branch}"), + seconds, + }, + None, + ); + let name: gix::refs::FullName = format!("refs/heads/{branch}") + .try_into() + .expect("valid refname"); + state + .refs + .transaction(&[gix_ref_store::RefEdit { + name, + expected: gix_ref_store::Expected::Any, + new: Some(oid), + }]) + .expect("moves the ref"); + oid +} + +/// A `Done`, `manual`-policy session with a result branch offers the +/// one-tap "Open review" form; posting it (`POST /agents/{id}/review`) +/// opens a review of the branch's own tip through the same signed web path +/// `crate::pages::commits::review` uses for a commit-page review, after +/// which the form no longer renders -- the review it would open already +/// exists (`docs/agent-sessions-plan.adoc`'s Phase 5 acceptance: "manual +/// yields none and the session page offers one-tap open"). +#[tokio::test] +// @relation(model.review, model.review-pin, roots.web-signing, roots.web-session, lens.parity, scope=function, role=Verifies) +async fn manual_session_offers_a_one_tap_open_review_form_that_opens_one() { + let seed = 30u8; + let state = build_state(FixtureIdentity { + name: "filer", + key: Keypair::from_seed(seed), + }); + let router = ents_web::router(state.clone()); + let id = seed_agent_via_web(&router, &state, "ship the fix").await; + + let key = Keypair::from_seed(seed); + let identity = Identity { + actor: fixture_actor("filer"), + author: None, + sign: &|payload| key.sign(payload), + }; + ents_forge::agent::revise_plan( + state.refs.as_ref(), + &*state.objects(), + state.events.as_ref(), + &id, + "do the thing".to_owned(), + &identity, + state.mode, + ) + .expect("revise_plan reaches an outcome"); + ents_forge::agent::confirm( + state.refs.as_ref(), + &*state.objects(), + state.events.as_ref(), + &id, + None, + &identity, + state.mode, + ) + .expect("confirm reaches an outcome"); + ents_forge::agent::claim( + state.refs.as_ref(), + &*state.objects(), + state.events.as_ref(), + &id, + ents_forge::agent::ClaimAgentSession { + worker: MemberId::new("worker-bot"), + sprite: "sprite-9".to_owned(), + }, + &identity, + state.mode, + ) + .expect("claim reaches an outcome"); + let branch = "agent/filer/deadbeef"; + ents_forge::agent::finish( + state.refs.as_ref(), + &*state.objects(), + state.events.as_ref(), + &id, + ents_forge::agent::FinishAgentSession { + outcome: ents_forge::agent::FinishOutcome::Done, + result_branch: Some(branch.to_owned()), + thread: vec![b"final log".to_vec()], + }, + &identity, + state.mode, + ) + .expect("finish reaches an outcome"); + advance_branch(&state, branch, 1_100); + + let detail = get_body(&router, &format!("/agents/{id}")).await; + assert!(detail.contains("done")); + assert!( + detail.contains("Open review"), + "a done, manual session with a result branch offers the one-tap open: {detail}" + ); + + let (cookie, csrf) = session_cookie_and_csrf(&router, &state, "/agents").await; + let response = router + .clone() + .oneshot( + Request::post(format!("/agents/{id}/review")) + .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!( + response.status().is_redirection(), + "{:?}", + response.status() + ); + + let after = get_body(&router, &format!("/agents/{id}")).await; + assert!( + !after.contains("Open review"), + "once opened, the one-tap form must not render again" + ); +} + +/// `POST /agents/{id}/review` without a valid CSRF token is rejected -- +/// this is a state-changing route like every other signed mutation +/// (`roots.web-session`). +#[tokio::test] +// @relation(roots.web-session, scope=function, role=Verifies) +async fn open_review_is_rejected_without_a_valid_csrf_token() { + let seed = 31u8; + let state = build_state(FixtureIdentity { + name: "filer", + key: Keypair::from_seed(seed), + }); + let router = ents_web::router(state.clone()); + let id = seed_agent_via_web(&router, &state, "ship the fix").await; + + let (cookie, _csrf) = session_cookie_and_csrf(&router, &state, "/agents").await; + let response = router + .clone() + .oneshot( + Request::post(format!("/agents/{id}/review")) + .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!(response.status(), StatusCode::BAD_REQUEST); +} + // --------------------------------------------------------------------- // `crate::pages::agent_chat` (`docs/agent-sessions-plan.adoc`'s Phase 4: // the laptop planning-chat page).

crates/cli/git-ents/src/hook.rs @@ -280,6 +280,23 @@ &|payload| signer.sign(payload), Mode::Mandatory, )?; + } else if effect_name == crate::review_worker::AGENT_REVIEW_NAME { + // The `agent-review` effect (`docs/agent-sessions-plan.adoc`'s + // Phase 5) needs its own bespoke handling too, but for the + // opposite reason `agent-exec`/`agent-plan` do: it runs no + // sandboxed command at all (`crate::review_worker`'s own doc), + // so `executor`/`toolchains`/`effect.run` — resolved above for + // every effect uniformly — are simply unused for this one. + crate::review_worker::run_agent_review( + &root.refs, + &root.objects, + &root.events, + oid, + ents_model::MemberId::new(crate::root::HOSTED_WORKER_NAME), + &author, + &|payload| signer.sign(payload), + Mode::Mandatory, + )?; } else { run_one( &root.refs,

crates/cli/git-ents/src/lib.rs @@ -82,6 +82,7 @@ pub mod mutate; pub mod package; pub mod plan_worker; +pub mod review_worker; pub mod root; pub mod sign;

crates/kernel/ents-effect/src/definition.rs @@ -144,6 +144,55 @@ } } +/// The canonical `agent-review` effect's own name +/// (`docs/agent-sessions-plan.adoc`'s Phase 5, "Auto-open is a follow-on +/// effect") — the final segment of `refs/meta/effects/agent-review`. +pub const AGENT_REVIEW_NAME: &str = "agent-review"; + +/// `agent-review`'s trigger: every `agent-exec` result recorded `pass` +/// (`query.results`) — the follow-on's own words, "subscribed via +/// `results(agent-exec)`," resolved to `query.grammar`'s actual two-argument +/// `results(effect, status)` form. Only a `pass` result is a completed run +/// with a result branch to review; a `fail`/`error` result names a run that +/// never reached `Done`, for which there is nothing to open a review of. +/// This module's own tests pin the exact syntax against the real parser, +/// mirroring [`AGENT_EXEC_TRIGGER`] and [`AGENT_PLAN_TRIGGER`]'s own tests. +pub const AGENT_REVIEW_TRIGGER: &str = "results(agent-exec, pass)"; + +/// The canonical `agent-review` [`Effect`] definition +/// (`docs/agent-sessions-plan.adoc`'s Phase 5): opening a review is pure +/// repository mutation (a signed commit onto the review's own entity ref +/// plus its retention pin) with no sandboxed command to run at all, unlike +/// [`agent_exec`] and [`agent_plan`] — so unlike those two constructors, +/// this one takes no `toolchains`/`run` parameters to fix: an effect +/// definition still carries the two fields (`model.effect-definition` +/// requires them of every effect), but this handler +/// (`git_ents::review_worker::run_agent_review`) never resolves a toolchain +/// or invokes an [`ents_effect`]-crate `Executor` for it, so there is +/// nothing meaningful a caller could fix them to. +/// +/// # Examples +/// +/// ``` +/// use ents_effect::definition::{agent_review, validate}; +/// +/// let effect = agent_review(); +/// assert_eq!(effect.name, "agent-review"); +/// assert!(effect.toolchains.is_empty()); +/// validate(&effect).expect("the canonical trigger validates"); +/// ``` +#[must_use] +pub fn agent_review() -> Effect { + Effect { + name: AGENT_REVIEW_NAME.to_owned(), + trigger: AGENT_REVIEW_TRIGGER.to_owned(), + toolchains: Vec::new(), + run: "no sandboxed command: agent-review is pure repository mutation, handled entirely \ + by its composition-root handler" + .to_owned(), + } +} + #[cfg(test)] mod tests { #![allow(clippy::expect_used, reason = "unit test")] @@ -254,4 +303,33 @@ assert_eq!(AGENT_PLAN_TRIGGER, AGENT_EXEC_TRIGGER); assert_ne!(AGENT_PLAN_NAME, AGENT_EXEC_NAME); } + + // ---- The canonical `agent-review` definition + // (`docs/agent-sessions-plan.adoc`'s Phase 5) ---- + + #[rstest] + // @relation(query.grammar, query.results, scope=function, role=Verifies) + fn agent_review_trigger_parses_against_the_real_query_grammar() { + let query: ents_query::Query = AGENT_REVIEW_TRIGGER + .parse() + .expect("the canonical agent-review trigger parses"); + assert_eq!(query.results_dependencies(), ["agent-exec"]); + } + + #[rstest] + // @relation(effect.validation, scope=function, role=Verifies) + fn agent_review_definition_validates() { + let effect = agent_review(); + assert_eq!(effect.name, AGENT_REVIEW_NAME); + assert!(effect.toolchains.is_empty()); + validate(&effect).expect("the canonical agent-review definition validates"); + } + + #[rstest] + // @relation(scope=function, role=Verifies) + fn agent_review_is_downstream_of_agent_exec_pass_only() { + assert!(AGENT_REVIEW_TRIGGER.contains("pass")); + assert_ne!(AGENT_REVIEW_NAME, AGENT_EXEC_NAME); + assert_ne!(AGENT_REVIEW_NAME, AGENT_PLAN_NAME); + } }

crates/cli/ents-web/src/pages/agents.rs @@ -175,6 +175,7 @@ // navigation chrome, never a reason to fail the session's own page. let (rows, _unreadable) = agent::list_all(state.refs.as_ref(), &*state.objects()).unwrap_or_default(); + let offer_manual_review = manual_review_still_open(&state, &agent_session); Ok(super::layout_split( &super::RepoHeader::from_state(&state), @@ -234,6 +235,9 @@ @if agent_session.awaiting_confirmation() { (confirm_form(&session, &id)) } + @if offer_manual_review { + (open_review_form(&session, &id)) + } } h2 { "Timeline" } (timeline_section(&timeline)) @@ -529,6 +533,164 @@ csrf: String, } +/// Whether [`show`] should offer the manual one-tap "Open review" form +/// (`docs/agent-sessions-plan.adoc`'s Phase 5, "manual yields none ... and +/// the session page offers one-tap open"): the session is `Done` with a +/// result branch, its resolved review policy (the confirm's own frozen +/// value, falling back to [`ents_forge::agent::SessionMeta::review_policy`] +/// for the same defensive reason [`ents_forge::agent::dispatch_review`] +/// itself consults the confirm first) is +/// [`ReviewPolicy::Manual`], and no review of that result branch's current +/// tip exists yet under the viewing identity's own resolved member -- +/// mirroring [`ents_forge::agent::dispatch_review`]'s own precondition on +/// the auto-open side, so the two paths never both claim (or both refuse) +/// the same session. +fn manual_review_still_open<O: Find>(state: &AppState<O>, agent_session: &AgentSession) -> bool { + if agent_session.meta.status != SessionStatus::Done { + return false; + } + let resolved_policy = agent_session + .confirm + .as_ref() + .map_or(agent_session.meta.review_policy, |confirm| { + confirm.review_policy + }); + if resolved_policy != ReviewPolicy::Manual { + return false; + } + let Some(branch) = &agent_session.meta.result_branch else { + return false; + }; + let Some(target_hex) = branch_tip_hex(state, branch) else { + return false; + }; + let member = session_owner(state); + !review_exists(state, &target_hex, &member) +} + +/// The hex oid of `branch`'s current `refs/heads/<branch>` tip, or `None` +/// when that ref does not resolve in this ref store -- the same lookup +/// [`result_branch_cell`] performs, factored out so +/// [`manual_review_still_open`] can reuse it without also building that +/// cell's own markup. +fn branch_tip_hex<O: Find>(state: &AppState<O>, branch: &str) -> Option<String> { + let name: gix::refs::FullName = format!("refs/heads/{branch}").try_into().ok()?; + state + .refs + .get(name.as_ref()) + .ok() + .flatten() + .map(|oid| oid.to_string()) +} + +/// Whether a review of `target_hex` already exists under `member`'s own +/// composite key (`model.review`) -- a plain ref-existence check, since the +/// manual-open path only ever targets a session's result branch at its +/// current (post-`Done`, immutable) tip, never a moving target a +/// fast-forward re-review would need to resolve ancestry for. +fn review_exists<O: Find>( + state: &AppState<O>, + target_hex: &str, + member: &ents_model::MemberId, +) -> bool { + ents_model::namespace::review_ref(target_hex, member) + .ok() + .and_then(|name| state.refs.get(name.as_ref()).ok().flatten()) + .is_some() +} + +/// The one-tap "Open review" form (`POST /agents/{id}/review`): rendered +/// only while [`manual_review_still_open`] holds -- a plain button, no +/// fields but the CSRF token, mirroring [`confirm_form`]'s identical +/// one-tap shape so opening a manual review from a phone is a single tap +/// too (`docs/agent-sessions-plan.adoc`'s Phase 5). +fn open_review_form(session: &Session, id: &str) -> Markup { + html! { + form method="post" action=(format!("/agents/{id}/review")) { + (super::csrf_input(session)) + button type="submit" { "Open review" } + } + } +} + +/// `POST /agents/{id}/review`: open a review of `id`'s result branch's +/// current tip -- the manual counterpart of the `agent-review` effect's own +/// auto-open (`git_ents::review_worker::run_agent_review`), writing through +/// the identical primitive both that effect and +/// `crate::pages::commits::review` ultimately share +/// ([`ents_receive::propose_entity_with_pin`], `receive.multi-ref-atomicity`) +/// rather than `ents_forge::review::new`'s own repo-path rev-resolution: +/// the branch tip this page already resolved to decide whether to offer +/// the form ([`branch_tip_hex`]) is read off this same ref store, so the +/// commit a POST here reviews is guaranteed to be the exact commit the GET +/// that rendered the form showed -- no second, possibly divergent +/// resolution of the same branch name. The opened review carries +/// [`ents_forge::review::Verdict::Comment`] and a body naming the session, +/// exactly like the effect's own auto-opened review -- judgment is left to +/// the reviewer's first comment on it, not this one-tap action. +/// +/// # Errors +/// +/// [`crate::Error::BadCsrf`] if `form.csrf` does not match; +/// [`crate::Error::InvalidArgument`] if the session has no result branch, or +/// that branch's own ref does not resolve; otherwise propagates +/// [`ents_receive::propose_entity_with_pin`]'s own failures. +// @relation(model.review, model.review-pin, receive.multi-ref-atomicity, roots.web-signing, roots.web-session, scope=function) +pub async fn open_review<O>( + State(state): State<Arc<AppState<O>>>, + axum::Extension(session): axum::Extension<Session>, + Path(id): Path<String>, + Form(form): Form<OpenReviewForm>, +) -> Result<impl IntoResponse> +where + O: Find + Write + Send + 'static, +{ + super::require_csrf(&session, &form.csrf)?; + let agent_session = agent::show(state.refs.as_ref(), &*state.objects(), &id)?; + let branch = agent_session.meta.result_branch.clone().ok_or_else(|| { + Error::InvalidArgument(format!("agent session {id} has no result branch to review")) + })?; + let target_hex = branch_tip_hex(&state, &branch).ok_or_else(|| { + Error::InvalidArgument(format!( + "agent session {id}'s result branch {branch:?} does not resolve" + )) + })?; + let target = ObjectId::from_hex(target_hex.as_bytes()).map_err(|_source| { + Error::InvalidArgument(format!("not a well-formed oid: {target_hex}")) + })?; + let member = session_owner(&state); + let review_ref = ents_model::namespace::review_ref(&target_hex, &member)?; + let pin_ref = ents_model::namespace::review_pin_ref(&target_hex, &member)?; + let review = ents_forge::review::Review::new( + target, + ents_forge::review::Verdict::Comment, + format!("Review opened for agent session {id}."), + ); + let identity = state.identity.as_ref(); + let outcome = ents_receive::propose_entity_with_pin( + state.refs.as_ref(), + &*state.objects(), + state.events.as_ref(), + review_ref, + &review, + pin_ref, + target, + &crate::receive_identity!(identity, crate::pages::member_author(&session)), + &format!("Open review of agent session {id}'s result"), + &format!("Pin review {target_hex}/{member}"), + state.mode, + )?; + crate::error::outcome_to_result(outcome)?; + Ok(Redirect::to(&format!("/agents/{id}"))) +} + +/// The form fields `POST /agents/{id}/review` accepts. +#[derive(Debug, Deserialize)] +pub struct OpenReviewForm { + /// The per-session CSRF token (`roots.web-session`). + csrf: String, +} + /// 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

crates/forge/ents-forge/src/agent/dispatch.rs @@ -14,7 +14,7 @@ //! the wrong direction either, so this stays a same-crate function next to //! the type it decides over, with no new cross-crate dependency at all. -use super::{AgentSession, Status}; +use super::{AgentSession, ReviewPolicy, Status}; /// What a dequeued `(agent-exec, oid)` pair resolves to once the runner /// reads the agent session tip at `oid`. @@ -119,6 +119,67 @@ } } +/// What a dequeued `(agent-review, oid)` pair resolves to once the runner +/// reads the agent session tip at `oid` (`docs/agent-sessions-plan.adoc`'s +/// Phase 5): auto-review opens a follow-on review iff the session reached +/// [`Status::Done`] with a result branch recorded and its +/// [`super::Confirm::review_policy`] — the policy actually frozen at +/// confirm time, never [`super::SessionMeta::review_policy`]'s own +/// possibly-since-changed value — is [`ReviewPolicy::Auto`]. Everything +/// else (`Manual`, no result branch, or a `Done` session that somehow +/// carries no confirm at all, unreachable through +/// [`super::command::confirm`] but not through this predicate) is a cheap +/// `pass` no-op, review-opening left to the member +/// (`docs/agent-sessions-plan.adoc`'s Phase 5, "manual yields none"). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ReviewDispatch { + /// The runner should open (or confirm it already opened, idempotency + /// judged downstream by the review's own deterministic refname) a + /// review of the session's result branch. + Open, + /// Anything else: a cheap `pass` no-op, no review opened. + NoOp, +} + +/// Decide [`ReviewDispatch`] for `session`'s current tip. +/// +/// # Examples +/// +/// ``` +/// use ents_forge::agent::{Confirm, ReviewDispatch, ReviewPolicy, SessionMeta, dispatch_review}; +/// use ents_model::MemberId; +/// +/// let mut session = ents_forge::agent::AgentSession { +/// meta: SessionMeta::new( +/// MemberId::new("jdc"), 1_000, "claude-sonnet-5", vec![], +/// "refs/heads/main", ReviewPolicy::Auto, None, +/// ), +/// plan: Some("do the thing".to_owned()), +/// confirm: Some(Confirm::new( +/// gix_hash::ObjectId::from_bytes_or_panic(&[7u8; 20]), +/// ReviewPolicy::Auto, +/// )), +/// thread: vec![], +/// }; +/// session.meta.status = ents_forge::agent::Status::Done; +/// assert_eq!(dispatch_review(&session), ReviewDispatch::NoOp, "no result branch yet"); +/// +/// session.meta.result_branch = Some("agent/jdc/abc1234".to_owned()); +/// assert_eq!(dispatch_review(&session), ReviewDispatch::Open); +/// ``` +#[must_use] +pub fn dispatch_review(session: &AgentSession) -> ReviewDispatch { + let auto = session + .confirm + .as_ref() + .is_some_and(|confirm| confirm.review_policy == ReviewPolicy::Auto); + if session.meta.status == Status::Done && session.meta.result_branch.is_some() && auto { + ReviewDispatch::Open + } else { + ReviewDispatch::NoOp + } +} + #[cfg(test)] mod tests { #![allow(clippy::expect_used, reason = "unit test")] @@ -152,13 +213,20 @@ /// here since that accessor is the only way this test crosses into it /// (mirroring `entity`'s own `blob_hash` test helper). fn confirm_for(plan: &str) -> Confirm { + confirm_with(plan, ReviewPolicy::Manual) + } + + /// [`confirm_for`], generalized over the frozen review policy -- + /// `dispatch_review`'s own tests need both variants, not just + /// `Manual`. + fn confirm_with(plan: &str, policy: ReviewPolicy) -> Confirm { let hash = gix_object::compute_hash( gix_hash::Kind::Sha1, gix_object::Kind::Blob, plan.as_bytes(), ) .expect("hashing an in-memory byte slice cannot fail"); - Confirm::new(hash, ReviewPolicy::Manual) + Confirm::new(hash, policy) } #[rstest] @@ -245,4 +313,88 @@ already_planned.thread.push(b"fix the flaky test".to_vec()); assert_eq!(dispatch_plan(&already_planned), PlanDispatch::NoOp); } + + // --------------------------------------------------------------- + // `dispatch_review`: `docs/agent-sessions-plan.adoc`'s Phase 5. + // --------------------------------------------------------------- + + /// A `Done` session whose confirm froze `Auto` and carries a result + /// branch opens. + #[rstest] + // @relation(scope=function, role=Verifies) + fn dispatch_review_opens_a_done_auto_session_with_a_result_branch() { + let plan = "do the thing"; + let confirm = confirm_with(plan, ReviewPolicy::Auto); + let mut done = session(Status::Done, Some(plan), Some(confirm)); + done.meta.result_branch = Some("agent/jdc/abc1234".to_owned()); + assert_eq!(dispatch_review(&done), ReviewDispatch::Open); + } + + /// `Manual` never opens, whether or not a result branch exists. + #[rstest] + // @relation(scope=function, role=Verifies) + fn dispatch_review_is_no_op_for_manual() { + let plan = "do the thing"; + let confirm = confirm_with(plan, ReviewPolicy::Manual); + let mut done = session(Status::Done, Some(plan), Some(confirm)); + done.meta.result_branch = Some("agent/jdc/abc1234".to_owned()); + assert_eq!(dispatch_review(&done), ReviewDispatch::NoOp); + } + + /// `Auto`, but no result branch was ever recorded (a `Done` session + /// whose run somehow never pushed one) — nothing to review yet. + #[rstest] + // @relation(scope=function, role=Verifies) + fn dispatch_review_is_no_op_without_a_result_branch() { + let plan = "do the thing"; + let confirm = confirm_with(plan, ReviewPolicy::Auto); + let done = session(Status::Done, Some(plan), Some(confirm)); + assert!(done.meta.result_branch.is_none()); + assert_eq!(dispatch_review(&done), ReviewDispatch::NoOp); + } + + /// `Auto`, with a result branch, but not yet `Done` (still `Running`) -- + /// nothing to review before the run finishes. + #[rstest] + #[case::planning(Status::Planning)] + #[case::ready(Status::Ready)] + #[case::running(Status::Running)] + #[case::failed(Status::Failed(FailureReason { detail: "oops".to_owned() }))] + // @relation(scope=function, role=Verifies) + fn dispatch_review_is_no_op_outside_done(#[case] status: Status) { + let plan = "do the thing"; + let confirm = confirm_with(plan, ReviewPolicy::Auto); + let mut session = session(status, Some(plan), Some(confirm)); + session.meta.result_branch = Some("agent/jdc/abc1234".to_owned()); + assert_eq!(dispatch_review(&session), ReviewDispatch::NoOp); + } + + /// `Done`, a result branch recorded, but no confirm at all on the tip + /// (unreachable through the command layer, not through this predicate) + /// — no policy to read as `Auto`, so no open. + #[rstest] + // @relation(scope=function, role=Verifies) + fn dispatch_review_is_no_op_with_no_confirm_at_all() { + let mut done = session(Status::Done, Some("do the thing"), None); + done.meta.result_branch = Some("agent/jdc/abc1234".to_owned()); + assert_eq!(dispatch_review(&done), ReviewDispatch::NoOp); + } + + /// [`SessionMeta::review_policy`] having since drifted to `Auto` must + /// never override what [`Confirm::review_policy`] actually froze -- + /// only the confirm's own field is consulted. + #[rstest] + // @relation(scope=function, role=Verifies) + fn dispatch_review_reads_the_confirms_frozen_policy_not_the_metas_current_one() { + let plan = "do the thing"; + let confirm = confirm_with(plan, ReviewPolicy::Manual); + let mut done = session(Status::Done, Some(plan), Some(confirm)); + done.meta.result_branch = Some("agent/jdc/abc1234".to_owned()); + done.meta.review_policy = ReviewPolicy::Auto; + assert_eq!( + dispatch_review(&done), + ReviewDispatch::NoOp, + "meta.review_policy drifting to auto must not resurrect a manual confirm" + ); + } }

crates/forge/ents-forge/src/agent/mod.rs @@ -19,7 +19,9 @@ confirm, draft_plan, draft_plan_transition, finish, finish_transition, list, list_all, new, reopen, revise_plan, show, }; -pub use dispatch::{Dispatch, PlanDispatch, dispatch, dispatch_plan}; +pub use dispatch::{ + Dispatch, PlanDispatch, ReviewDispatch, dispatch, dispatch_plan, dispatch_review, +}; pub use entity::{ AgentSession, Confirm, FailureReason, ReviewPolicy, SessionMeta, Status, ToolchainPin, };

crates/cli/git-ents/src/review_worker.rs @@ -1,0 +1,669 @@ +//! The `agent-review` effect's run path (`docs/agent-sessions-plan.adoc`'s +//! Phase 5): auto-opening a follow-on [`ents_forge::review::Review`] once +//! an `agent-exec` run finishes `Done` with a result branch, for a session +//! whose confirm froze [`ents_forge::agent::ReviewPolicy::Auto`]. +//! +//! # Why this lives here, not in `ents-effect` or `ents-forge` +//! +//! Exactly [`crate::agent_worker`]'s and [`crate::plan_worker`]'s own +//! reasoning: `ents-effect` links exactly `ents-model`, `ents-query`, and +//! `ents-receive` (`docs/spec/overview.adoc`'s crate-graph table) — never +//! `ents-forge`, from either side — so a function that needs +//! [`ents_forge::agent`]'s typed session reads and +//! [`ents_forge::review::Review`]'s own shape at once cannot live in either +//! kernel crate. `git-ents` already depends on both, so this is the same +//! "session handler" composition-root seam [`crate::hook::post_receive`] +//! installs [`crate::agent_worker`] and [`crate::plan_worker`] for, +//! installed here for the one other effect name ([`AGENT_REVIEW_NAME`]) +//! that needs bespoke handling. +//! +//! # No sandbox at all, unlike `agent-exec` and `agent-plan` +//! +//! Opening a review is pure repository mutation — a signed commit onto the +//! review's own entity ref plus its retention pin +//! (`model.review`, `model.review-pin`) — never a sandboxed command. This +//! module's own [`run_agent_review`] takes no `Executor`, no toolchains, no +//! `run` command, and no scratch directory: unlike +//! [`crate::agent_worker::run_agent_exec`] and +//! [`crate::plan_worker::run_agent_plan`], there is nothing here for an +//! [`ents_effect::Executor`] to do. [`ents_effect::definition::agent_review`]'s +//! own doc explains why its canonical [`ents_model::Effect`] definition +//! still carries (empty/inert) `toolchains`/`run` fields despite that: the +//! shape every effect definition carries, never consulted by this handler. +//! +//! # What the dequeued oid already is +//! +//! `AGENT_REVIEW_TRIGGER`'s own `results(agent-exec, pass)` semantics +//! (`query.results`) resolve to the *tested* commit — the session tip +//! `agent_worker::run_agent_exec` read as its own dispatched oid — never +//! the result ref's own commit. This module never decodes the `agent-exec` +//! `ResultRecord` itself: the dequeued `oid` already *is* that tested +//! commit, so walking it to genesis ([`genesis_of`]) recovers the session +//! id directly, exactly as `agent_worker`'s and `plan_worker`'s own +//! identically-named private copies do. +//! +//! # Idempotency +//! +//! One result, one obligation +//! (`docs/agent-sessions-plan.adoc`'s Phase 5: "idempotent by +//! construction"): before ever proposing a review, this module checks +//! whether the exact review this effect would open — +//! `refs/meta/reviews/<result-branch-tip>/<reviewer>`, the same +//! deterministic composite key every review occupies +//! (`model.review`) — already exists, and no-ops if so. A worker retry +//! that re-dequeues the same `oid` a second time (this effect's own results +//! ref did not yet land before the process died) finds that ref already +//! there and takes the same no-op path, so re-running the handler twice +//! never yields a second review. + +use ents_forge::agent::{ReviewDispatch, dispatch_review}; +use ents_forge::review::{Review, Verdict}; +use ents_model::MemberId; +use ents_receive::{EventSink, Identity, Mode, Outcome, TxResult}; +use gix_hash::ObjectId; +use gix_object::{CommitRef, Find, Kind, Write}; +use gix_ref_store::RefStore; + +use crate::error::{Error, Result}; + +/// `agent-review`'s own effect name — re-exported so a caller deciding +/// which bespoke handler a dequeued `(effect, oid)` obligation belongs to +/// does not need a second import of `ents_effect::definition`. +pub const AGENT_REVIEW_NAME: &str = ents_effect::definition::AGENT_REVIEW_NAME; + +/// What running the `agent-review` effect against one dequeued +/// `(agent-review, oid)` obligation did. +#[derive(Debug)] +pub enum AgentReviewOutcome { + /// The session's confirm did not freeze `Auto`, or it has no result + /// branch recorded (`ReviewDispatch::NoOp`): a cheap `pass` was + /// recorded to discharge the obligation, no review opened. + NoOp, + /// The review this effect would open already exists — either an + /// earlier run of this same effect got as far as opening it before a + /// worker died short of recording its own result (the idempotency case + /// this module's own doc describes), or a manual reviewer opened one + /// under this same identity first: a `pass` was recorded, and no + /// second review commit was proposed. + AlreadyOpen { + /// The session's own genesis-oid id. + id: String, + }, + /// This worker opened the review, atomically with its retention pin. + Opened { + /// The session's own genesis-oid id. + id: String, + /// The result branch tip the opened review targets. + target: ObjectId, + /// The review-and-pin proposal's own outcome. + outcome: Outcome, + }, +} + +/// Run the `agent-review` effect against the single dequeued commit `oid` +/// — the tested commit `agent-exec`'s own `pass` result names (see this +/// module's own doc for why that already is the session tip to walk to +/// genesis, never the result ref's commit). +/// +/// `reviewer` becomes both the opened review's composite-key `<member>` +/// segment and the identity `sign`/`author` sign as +/// (`gate.owner-mutation`'s own rule for `Namespace::Review`: "a review +/// advances only under the signature of the member its refname names," with +/// no carve-out for genesis) — the same worker identity +/// [`crate::agent_worker::run_agent_exec`]'s own `worker` parameter signs +/// the session's claim and finish as. +/// +/// # Errors +/// +/// Any [`Error`] from reading or decoding the session, resolving the result +/// branch's own ref, or building and sending the review-and-pin proposal. +#[expect( + clippy::too_many_arguments, + reason = "one input per identity/materialization step, mirrors run_agent_exec's and \ + run_agent_plan's own identically-justified shape, minus the sandbox-only \ + parameters this effect never needs" +)] +pub fn run_agent_review<O>( + refs: &dyn RefStore, + objects: &O, + events: &dyn EventSink, + oid: ObjectId, + reviewer: MemberId, + author: &gix::actor::Signature, + sign: &dyn Fn(&[u8]) -> String, + mode: Mode, +) -> Result<AgentReviewOutcome> +where + O: Find + Write, +{ + let results_ref = + ents_model::namespace::result_ref(AGENT_REVIEW_NAME, &ents_effect::run::short_oid(oid))?; + let id = genesis_of(objects, oid)?.to_string(); + let session = ents_forge::agent::show(refs, objects, &id)?; + + if dispatch_review(&session) == ReviewDispatch::NoOp { + record_pass(refs, objects, events, &results_ref, oid, author, sign, mode)?; + return Ok(AgentReviewOutcome::NoOp); + } + + // `ReviewDispatch::Open` guarantees a result branch is recorded. + let branch_name = session.meta.result_branch.clone().ok_or_else(|| { + Error::InvalidArgument(format!( + "agent session {id} dispatched to open a review with no result branch recorded" + )) + })?; + let branch_ref: gix::refs::FullName = + format!("refs/heads/{branch_name}") + .try_into() + .map_err(|_source| { + Error::InvalidArgument(format!( + "agent session {id}'s result branch {branch_name:?} is not a well-formed \ + refname" + )) + })?; + let target = refs + .get(branch_ref.as_ref())? + .ok_or_else(|| Error::NotFound { + what: branch_name.clone(), + })?; + + let review_ref = ents_model::namespace::review_ref(&target.to_string(), &reviewer)?; + if refs.get(review_ref.as_ref())?.is_some() { + record_pass(refs, objects, events, &results_ref, oid, author, sign, mode)?; + return Ok(AgentReviewOutcome::AlreadyOpen { id }); + } + + let pin_ref = ents_model::namespace::review_pin_ref(&target.to_string(), &reviewer)?; + let review = Review::new( + target, + Verdict::Comment, + format!( + "Auto-opened by the agent-review effect: agent session {id}'s agent-exec run \ + landed at {oid}." + ), + ); + let identity = Identity { + actor: author.clone(), + author: None, + sign, + }; + let outcome = ents_receive::propose_entity_with_pin( + refs, + objects, + events, + review_ref, + &review, + pin_ref, + target, + &identity, + &format!("Auto-open review of agent session {id}'s result"), + &format!("Pin review {target}/{reviewer}"), + mode, + )?; + if outcome.result != TxResult::Applied { + // Lost a race against another writer that opened the identical + // review between this worker's own pre-check above and this write + // — discharge the obligation exactly like the already-open case, + // never propose a second, conflicting review commit. + record_pass(refs, objects, events, &results_ref, oid, author, sign, mode)?; + return Ok(AgentReviewOutcome::AlreadyOpen { id }); + } + + record_pass(refs, objects, events, &results_ref, oid, author, sign, mode)?; + Ok(AgentReviewOutcome::Opened { + id, + target, + outcome, + }) +} + +/// Record a cheap `pass` for `oid` on the canonical `agent-review` results +/// ref — the no-op and already-open paths, and the discharge that follows +/// a successful open, all funnel through this one call. +#[expect( + clippy::too_many_arguments, + reason = "one input per write_result parameter; a thin, single-call wrapper, mirroring \ + agent_worker's and plan_worker's identically-shaped copies" +)] +fn record_pass<O: Find + Write>( + refs: &dyn RefStore, + objects: &O, + events: &dyn EventSink, + results_ref: &gix::refs::FullName, + oid: ObjectId, + author: &gix::actor::Signature, + sign: &dyn Fn(&[u8]) -> String, + mode: Mode, +) -> Result<Outcome> { + Ok(ents_effect::write_result( + refs, + objects, + events, + results_ref.clone(), + AGENT_REVIEW_NAME, + oid, + ents_model::Status::Pass, + author, + sign, + mode, + )?) +} + +/// The oldest ancestor of `oid` reachable by following each commit's first +/// parent — the session's own genesis oid and id; duplicated from +/// `agent_worker`'s own private copy (that module's own doc names this +/// codebase's accepted pattern for a small per-module copy). +fn genesis_of(objects: &impl Find, oid: ObjectId) -> Result<ObjectId> { + let mut current = oid; + loop { + let mut buf = Vec::new(); + let data = objects + .try_find(&current, &mut buf) + .map_err(|source| Error::InvalidArgument(source.to_string()))? + .ok_or_else(|| Error::NotFound { + what: current.to_string(), + })?; + if data.kind != Kind::Commit { + return Err(Error::NotFound { + what: current.to_string(), + }); + } + let commit = CommitRef::from_bytes(data.data, current.kind()) + .map_err(|source| Error::InvalidArgument(source.to_string()))?; + match commit.parents().next() { + Some(parent) => current = parent, + None => return Ok(current), + } + } +} + +#[cfg(test)] +mod tests { + #![allow( + clippy::expect_used, + clippy::unwrap_used, + clippy::panic, + reason = "unit test; the panic is an assertion on a `let else` branch" + )] + + use ents_forge::agent::{ClaimAgentSession, FinishAgentSession, FinishOutcome, ReviewPolicy}; + use ents_gate::Config; + use ents_model::{Provenance, namespace}; + use ents_receive::NullEventSink; + use ents_testutil::{Keypair, MemRefStore, ObjectStore, advance_ref, enroll_member}; + use gix_ref_store::RefStoreRead as _; + use rstest::rstest; + + use super::*; + + type Signer = Box<dyn Fn(&[u8]) -> String>; + + struct Fixture { + refs: MemRefStore, + objects: ObjectStore, + sign: Signer, + } + + impl Fixture { + fn new() -> Self { + let refs = MemRefStore::default(); + let objects = ObjectStore::default(); + let key = Keypair::from_seed(1); + let sign = Keypair::from_seed(1); + enroll_member( + &refs, + &objects, + "worker", + &key, + Provenance::AdminRegistered, + 100, + ); + let config_ref: gix::refs::FullName = namespace::CONFIG_REF.try_into().expect("valid"); + ents_testutil::write_meta_entity( + &refs, + &objects, + config_ref, + &Config { + epoch: Some(150), + ..Config::default() + }, + Some(&key), + 150, + ); + advance_ref(&refs, &objects, "refs/heads/main", 1, 200); + Self { + refs, + objects, + sign: Box::new(move |payload: &[u8]| sign.sign(payload)), + } + } + + fn identity(&self) -> Identity<'_> { + Identity { + actor: self.author(), + author: None, + sign: &*self.sign, + } + } + + fn author(&self) -> gix::actor::Signature { + gix::actor::Signature { + name: "worker".into(), + email: "worker@ents.test".into(), + time: gix::date::Time { + seconds: 1_000, + offset: 0, + }, + } + } + + fn sign_fn(&self) -> &dyn Fn(&[u8]) -> String { + &*self.sign + } + + fn reviewer() -> MemberId { + MemberId::new("worker") + } + + /// A confirmed session, driven through claim and finish to `Done` + /// with a real, pushed result branch — [`run_agent_review`]'s own + /// `ReviewDispatch::Open` precondition, parameterized by the + /// review policy frozen at confirm. + fn done_session(&self, policy: ReviewPolicy, branch: &str) -> (ObjectId, String) { + let identity = self.identity(); + let (id, outcome) = ents_forge::agent::new( + &self.refs, + &self.objects, + &NullEventSink, + ents_forge::agent::NewAgentSession { + member: MemberId::new("jdc"), + prompt: "fix the flaky test".to_owned(), + model: "claude-sonnet-5".to_owned(), + toolchains: vec![], + base_ref: "refs/heads/main".to_owned(), + review_policy: policy, + retry_of: None, + }, + &identity, + Mode::Advisory, + ) + .expect("creates"); + assert_eq!(outcome.result, TxResult::Applied); + + ents_forge::agent::revise_plan( + &self.refs, + &self.objects, + &NullEventSink, + &id, + "do the thing".to_owned(), + &identity, + Mode::Advisory, + ) + .expect("revises"); + ents_forge::agent::confirm( + &self.refs, + &self.objects, + &NullEventSink, + &id, + None, + &identity, + Mode::Advisory, + ) + .expect("confirms"); + + let ref_name = ents_model::namespace::agent_session_ref(&id).expect("valid"); + let queued_tip = self + .refs + .get(ref_name.as_ref()) + .expect("readable") + .expect("exists"); + + ents_forge::agent::claim( + &self.refs, + &self.objects, + &NullEventSink, + &id, + ClaimAgentSession { + worker: MemberId::new("worker"), + sprite: "sprite-1".to_owned(), + }, + &identity, + Mode::Advisory, + ) + .expect("claims"); + ents_forge::agent::finish( + &self.refs, + &self.objects, + &NullEventSink, + &id, + FinishAgentSession { + outcome: FinishOutcome::Done, + result_branch: Some(branch.to_owned()), + thread: vec![b"transcript".to_vec()], + }, + &identity, + Mode::Advisory, + ) + .expect("finishes"); + + // A real `refs/heads/<branch>` tip for the review to target -- + // `run_agent_review` resolves it directly off `refs`, never + // through a real on-disk `gix::open` (see this module's own + // doc: the review's `target` field is raw bytes, never an + // object this store must itself carry). + advance_ref( + &self.refs, + &self.objects, + &format!("refs/heads/{branch}"), + 1, + 300, + ); + + (queued_tip, id) + } + } + + #[rstest] + // @relation(scope=function, role=Verifies) + fn a_manual_session_is_a_cheap_no_op() { + let fixture = Fixture::new(); + let (oid, id) = fixture.done_session(ReviewPolicy::Manual, "agent/jdc/deadbee1"); + + let author = fixture.author(); + let run = run_agent_review( + &fixture.refs, + &fixture.objects, + &NullEventSink, + oid, + Fixture::reviewer(), + &author, + fixture.sign_fn(), + Mode::Advisory, + ) + .expect("runs"); + assert!(matches!(run, AgentReviewOutcome::NoOp)); + + let results_ref = + ents_model::namespace::result_ref(AGENT_REVIEW_NAME, &ents_effect::run::short_oid(oid)) + .expect("valid"); + assert!( + fixture + .refs + .get(results_ref.as_ref()) + .expect("readable") + .is_some(), + "the no-op path must still discharge the obligation with a recorded pass" + ); + + let session = ents_forge::agent::show(&fixture.refs, &fixture.objects, &id).expect("shows"); + let branch_ref: gix::refs::FullName = format!( + "refs/heads/{}", + session.meta.result_branch.expect("branch recorded") + ) + .try_into() + .expect("valid"); + let target_hex = format!( + "{}", + fixture + .refs + .get(branch_ref.as_ref()) + .expect("readable") + .expect("branch tip exists") + ); + let review_ref = + ents_model::namespace::review_ref(&target_hex, &Fixture::reviewer()).expect("valid"); + assert!( + fixture + .refs + .get(review_ref.as_ref()) + .expect("readable") + .is_none(), + "manual must never open a review" + ); + } + + #[rstest] + // @relation(model.review, model.review-pin, receive.multi-ref-atomicity, scope=function, role=Verifies) + fn an_auto_session_opens_exactly_one_review() { + let fixture = Fixture::new(); + let (oid, id) = fixture.done_session(ReviewPolicy::Auto, "agent/jdc/deadbee2"); + + let author = fixture.author(); + let run = run_agent_review( + &fixture.refs, + &fixture.objects, + &NullEventSink, + oid, + Fixture::reviewer(), + &author, + fixture.sign_fn(), + Mode::Advisory, + ) + .expect("runs"); + let AgentReviewOutcome::Opened { + id: opened_id, + target, + outcome, + } = run + else { + panic!("expected Opened, got {run:?}"); + }; + assert_eq!(opened_id, id); + assert_eq!(outcome.result, TxResult::Applied); + + let review_ref = + ents_model::namespace::review_ref(&target.to_string(), &Fixture::reviewer()) + .expect("valid"); + let review_tip = fixture + .refs + .get(review_ref.as_ref()) + .expect("readable") + .expect("review landed"); + let tree = crate::commands::commit_tree(&fixture.objects, review_tip).expect("tree"); + let review: ents_forge::review::Review = + facet_git_tree::deserialize(&tree, &fixture.objects).expect("decodes"); + assert_eq!(review.target(), target); + assert_eq!(review.verdict, ents_forge::review::Verdict::Comment); + + let pin_ref = + ents_model::namespace::review_pin_ref(&target.to_string(), &Fixture::reviewer()) + .expect("valid"); + assert!( + fixture + .refs + .get(pin_ref.as_ref()) + .expect("readable") + .is_some(), + "the retention pin must land atomically with the review" + ); + + let results_ref = + ents_model::namespace::result_ref(AGENT_REVIEW_NAME, &ents_effect::run::short_oid(oid)) + .expect("valid"); + assert!( + fixture + .refs + .get(results_ref.as_ref()) + .expect("readable") + .is_some(), + "opening a review must still discharge this effect's own obligation" + ); + } + + /// Running the handler twice on the same dequeued oid (a worker retry) + /// must yield exactly one review -- `docs/agent-sessions-plan.adoc`'s + /// Phase 5 acceptance, and the red test for this module's own + /// idempotency claim. + #[rstest] + // @relation(scope=function, role=Verifies) + fn a_worker_retry_yields_exactly_one_review() { + let fixture = Fixture::new(); + let (oid, _id) = fixture.done_session(ReviewPolicy::Auto, "agent/jdc/deadbee3"); + let author = fixture.author(); + + let first = run_agent_review( + &fixture.refs, + &fixture.objects, + &NullEventSink, + oid, + Fixture::reviewer(), + &author, + fixture.sign_fn(), + Mode::Advisory, + ) + .expect("runs"); + assert!(matches!(first, AgentReviewOutcome::Opened { .. })); + + let second = run_agent_review( + &fixture.refs, + &fixture.objects, + &NullEventSink, + oid, + Fixture::reviewer(), + &author, + fixture.sign_fn(), + Mode::Advisory, + ) + .expect("a retry is not an error"); + let AgentReviewOutcome::AlreadyOpen { .. } = second else { + panic!("expected AlreadyOpen on retry, got {second:?}"); + }; + + let session = + ents_forge::agent::show(&fixture.refs, &fixture.objects, &_id).expect("shows"); + let branch_ref: gix::refs::FullName = format!( + "refs/heads/{}", + session.meta.result_branch.expect("branch recorded") + ) + .try_into() + .expect("valid"); + let target = fixture + .refs + .get(branch_ref.as_ref()) + .expect("readable") + .expect("branch tip exists"); + let review_ref = + ents_model::namespace::review_ref(&target.to_string(), &Fixture::reviewer()) + .expect("valid"); + + // Exactly one review commit exists: the ref's own tip has no + // parent (a fresh genesis, never a second commit stacked onto it). + let review_tip = fixture + .refs + .get(review_ref.as_ref()) + .expect("readable") + .expect("review landed"); + let mut buf = Vec::new(); + let data = fixture + .objects + .try_find(&review_tip, &mut buf) + .expect("readable") + .expect("exists"); + let commit = CommitRef::from_bytes(data.data, review_tip.kind()).expect("decodes"); + assert_eq!( + commit.parents().count(), + 0, + "a retry must never stack a second commit onto the review ref" + ); + } +}

effect, forge, cli, web: the auto-review follow-on effect (phase 5)

Reviews

Start a review