diff --git a/Cargo.lock b/Cargo.lock index 1ef95832..c9d98e51 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -881,7 +881,7 @@ checksum = "f2032f911046de80f0a198e0901378627c33f59ea0ac00e363d481118bd70a53" [[package]] name = "automonique-platform-client" version = "0.1.0" -source = "git+https://github.com/bext-stack/automonique?rev=c1e772e37371ba00825923f8f35ffc652c0a4e95#c1e772e37371ba00825923f8f35ffc652c0a4e95" +source = "git+https://github.com/bext-stack/automonique?rev=7d2fad53558ccb38ef3fdb12945974f75a0e73ce#7d2fad53558ccb38ef3fdb12945974f75a0e73ce" dependencies = [ "automonique-protocol", "base64 0.22.1", @@ -892,7 +892,7 @@ dependencies = [ [[package]] name = "automonique-protocol" version = "0.1.0" -source = "git+https://github.com/bext-stack/automonique?rev=c1e772e37371ba00825923f8f35ffc652c0a4e95#c1e772e37371ba00825923f8f35ffc652c0a4e95" +source = "git+https://github.com/bext-stack/automonique?rev=7d2fad53558ccb38ef3fdb12945974f75a0e73ce#7d2fad53558ccb38ef3fdb12945974f75a0e73ce" [[package]] name = "av-scenechange" diff --git a/Cargo.toml b/Cargo.toml index e715c13a..a47a6638 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -84,8 +84,8 @@ sha2 = "0.10" base64 = "0.22" ed25519-dalek = "2" criterion = "0.5" -automonique-platform-client = { git = "https://github.com/bext-stack/automonique", rev = "c1e772e37371ba00825923f8f35ffc652c0a4e95" } -automonique-protocol = { git = "https://github.com/bext-stack/automonique", rev = "c1e772e37371ba00825923f8f35ffc652c0a4e95" } +automonique-platform-client = { git = "https://github.com/bext-stack/automonique", rev = "7d2fad53558ccb38ef3fdb12945974f75a0e73ce" } +automonique-protocol = { git = "https://github.com/bext-stack/automonique", rev = "7d2fad53558ccb38ef3fdb12945974f75a0e73ce" } windows-sys = { version = "0.60", features = [ "Win32_Foundation", "Win32_Security", diff --git a/crates/shelldeck-core/locales/en.toml b/crates/shelldeck-core/locales/en.toml index 7eb78f64..9180932e 100644 --- a/crates/shelldeck-core/locales/en.toml +++ b/crates/shelldeck-core/locales/en.toml @@ -1217,7 +1217,12 @@ fleet.review.note_delete = "Delete" fleet.review.note_stale = "Superseded snapshot" fleet.review.note_failed = "The anchored note could not be stored." fleet.review.save_note = "Save the note" -fleet.review.batch_blocked = "%{count} note(s) selected and retained. Batch delivery to the authorized session stays unavailable: the server advertises no capability for that action." +fleet.review.notes_selected = "%{count} note(s) selected and retained. A note is a local draft: prepare it as a comment before it can be delivered to the authorized session." +fleet.review.prepare_batch_send = "Send %{count} comment(s) to the session" +fleet.review.delivery_withheld_title = "Batch delivery unavailable" +fleet.review.delivery_withheld_no_server_capability = "No capability response is attributed to this project, workspace and snapshot revision: nothing was advertised, so no send control is offered." +fleet.review.delivery_withheld_no_deliverable_comment = "The server advertises no deliverable comment for this snapshot: no authorized session is bound to it, or no comment is in a sendable state. The advertisement is re-read on every refresh, including after a send." +fleet.review.delivery_withheld_no_custody_lane = "Durable custody is unavailable in this process: no batch send control is offered." fleet.review.attention_title = "Attention chronology" fleet.review.confirm_comment = "In project %{project}, workspace %{workspace}: add “%{body}” on %{file} · %{hunk} · %{side}:%{line}, from exact snapshot %{revision}." fleet.review.confirm_approval = "In project %{project}, workspace %{workspace}: approve review rev %{review_revision}, from exact snapshot %{revision}." @@ -1233,6 +1238,7 @@ fleet.review.pending_detail = "Action dispatched once; later attempts only look fleet.review.invalid_preview = "The action no longer matches the exact active review." fleet.review.never_dispatched = "The preview prepared before restart was never sent." fleet.review.capability_stale = "The rerun capability changed; refresh and confirm a new preview." +fleet.review.delivery_capability_stale = "The delivery advertisement changed; reload, then prepare a new selection." fleet.review.custody_failed = "Durable custody refused dispatch; no provider call started." fleet.review.receipt_actor = "%{detail} · actor %{actor}" fleet.review.receipt_actor_short = "actor %{actor}" @@ -1244,7 +1250,7 @@ fleet.review.lane_partial = "partially staged" fleet.review.staging_proposed = "%{action} · proposal %{id}" fleet.review.staging_blocked = "%{action} · blocked · proposal %{id}" fleet.review.staging_withheld_title = "Staging is read-only" -fleet.review.staging_withheld_no_server_capability = "The server advertises no staging capability for this snapshot: Platform v2 publishes only the check rerun. Staged, unstaged, untracked and conflict state stay visible, with no staging control." +fleet.review.staging_withheld_no_server_capability = "The server advertises no staging capability for this snapshot: Platform v2 defines neither a staging confirmation digest nor a receipt correlation, and hunk-level granularity is not representable in it. Staged, unstaged, untracked and conflict state stay visible, with no staging control." fleet.review.staging_withheld_no_custody_lane = "Durable custody is unavailable in this process: no staging control is offered." fleet.review.preview_text = "Sanitized text preview" fleet.review.preview_image = "Image %{media} · %{width}×%{height} px · %{bytes} bytes" diff --git a/crates/shelldeck-core/locales/fr.toml b/crates/shelldeck-core/locales/fr.toml index d6cc136f..4674ea19 100644 --- a/crates/shelldeck-core/locales/fr.toml +++ b/crates/shelldeck-core/locales/fr.toml @@ -1217,7 +1217,12 @@ fleet.review.note_delete = "Supprimer" fleet.review.note_stale = "Instantané dépassé" fleet.review.note_failed = "La note ancrée n’a pas pu être enregistrée." fleet.review.save_note = "Enregistrer la note" -fleet.review.batch_blocked = "%{count} note(s) sélectionnée(s) et conservées. L’envoi groupé vers la session autorisée reste indisponible : le serveur n’annonce aucune capacité pour cette action." +fleet.review.notes_selected = "%{count} note(s) sélectionnée(s) et conservées. Une note est un brouillon local : préparez-la en commentaire pour qu’elle devienne livrable à la session autorisée." +fleet.review.prepare_batch_send = "Envoyer %{count} commentaire(s) à la session" +fleet.review.delivery_withheld_title = "Envoi groupé indisponible" +fleet.review.delivery_withheld_no_server_capability = "Aucune réponse de capacité n’est attribuée à ce projet, cet espace et cette révision d’instantané : rien n’a été annoncé, donc aucune commande d’envoi n’est proposée." +fleet.review.delivery_withheld_no_deliverable_comment = "Le serveur n’annonce aucun commentaire livrable pour cet instantané : aucune session autorisée n’y est reliée, ou aucun commentaire n’est dans un état envoyable. L’annonce est relue à chaque rafraîchissement, y compris après un envoi." +fleet.review.delivery_withheld_no_custody_lane = "La garde durable est indisponible dans ce processus : aucune commande d’envoi groupé n’est proposée." fleet.review.attention_title = "Chronologie d’attention" fleet.review.confirm_comment = "Dans le projet %{project}, espace %{workspace} : ajouter « %{body} » sur %{file} · %{hunk} · %{side}:%{line}, depuis l’instantané exact %{revision}." fleet.review.confirm_approval = "Dans le projet %{project}, espace %{workspace} : approuver la rév. de revue %{review_revision}, depuis l’instantané exact %{revision}." @@ -1233,6 +1238,7 @@ fleet.review.pending_detail = "Action envoyée une seule fois ; les tentatives s fleet.review.invalid_preview = "L’action ne correspond plus exactement à la revue active." fleet.review.never_dispatched = "L’aperçu préparé avant le redémarrage n’a jamais été envoyé." fleet.review.capability_stale = "La capacité de relance a changé ; rechargez puis confirmez un nouvel aperçu." +fleet.review.delivery_capability_stale = "L’annonce de livraison a changé ; rechargez puis préparez une nouvelle sélection." fleet.review.custody_failed = "La garde durable a refusé l’envoi ; aucun appel fournisseur n’a commencé." fleet.review.receipt_actor = "%{detail} · acteur %{actor}" fleet.review.receipt_actor_short = "acteur %{actor}" @@ -1244,7 +1250,7 @@ fleet.review.lane_partial = "partiellement indexé" fleet.review.staging_proposed = "%{action} · proposition %{id}" fleet.review.staging_blocked = "%{action} · bloqué · proposition %{id}" fleet.review.staging_withheld_title = "Indexation en lecture seule" -fleet.review.staging_withheld_no_server_capability = "Le serveur n’annonce aucune capacité d’indexation pour cet instantané : Platform v2 ne publie que la relance de contrôle. L’état indexé, non indexé, non suivi et en conflit reste affiché, sans commande d’indexation." +fleet.review.staging_withheld_no_server_capability = "Le serveur n’annonce aucune capacité d’indexation pour cet instantané : Platform v2 ne définit ni digest de confirmation ni corrélation de reçu pour l’indexation, et la granularité au niveau du bloc n’y est pas représentable. L’état indexé, non indexé, non suivi et en conflit reste affiché, sans commande d’indexation." fleet.review.staging_withheld_no_custody_lane = "La garde durable est indisponible dans ce processus : aucune commande d’indexation n’est proposée." fleet.review.preview_text = "Aperçu texte assaini" fleet.review.preview_image = "Image %{media} · %{width}×%{height} px · %{bytes} octets" diff --git a/crates/shelldeck-core/src/config/platform.rs b/crates/shelldeck-core/src/config/platform.rs index a08acf46..daf59d78 100644 --- a/crates/shelldeck-core/src/config/platform.rs +++ b/crates/shelldeck-core/src/config/platform.rs @@ -1684,7 +1684,7 @@ mod tests { use automonique_protocol::platform_v2_transport::{ PlatformNegotiationResponse, PlatformV2Refusal, PlatformV2Request, PlatformV2Response, ReviewCapabilities, ReviewCheckRerunCapability, ReviewConfirmationDigest, - ReviewReceiptCorrelationDigest, + ReviewPullRequestCapabilities, ReviewReceiptCorrelationDigest, }; let snapshot = decode_review_snapshot(include_bytes!( @@ -1716,6 +1716,8 @@ mod tests { correlation.clone(), ) .unwrap()], + Vec::new(), + ReviewPullRequestCapabilities::default(), ) .unwrap(); let negotiated = || { @@ -1738,7 +1740,7 @@ mod tests { let mut capability_client = PlatformV2Client::new_testing(capability_transport); assert_eq!( load_review_capabilities(&mut capability_client, &target).unwrap(), - PlatformReviewCapabilitiesLoad::Available(capabilities.clone()) + PlatformReviewCapabilitiesLoad::Available(Box::new(capabilities.clone())) ); assert!(matches!( capability_client.transport().requests()[0].request(), diff --git a/crates/shelldeck-core/src/config/platform_review.rs b/crates/shelldeck-core/src/config/platform_review.rs index a0333a65..2183e68f 100644 --- a/crates/shelldeck-core/src/config/platform_review.rs +++ b/crates/shelldeck-core/src/config/platform_review.rs @@ -18,8 +18,8 @@ pub use automonique_protocol::platform_v2_review::{ ReviewSchemaVersion, ReviewSnapshot, ReviewText, WorktreeFileState, }; pub use automonique_protocol::platform_v2_transport::{ - ReviewCapabilities, ReviewCheckRerunCapability, ReviewConfirmationDigest, - ReviewReceiptCorrelationDigest, + ReviewAgentDeliveryCapability, ReviewCapabilities, ReviewCheckRerunCapability, + ReviewConfirmationDigest, ReviewPullRequestCapabilities, ReviewReceiptCorrelationDigest, }; use automonique_protocol::primitives::Revision; use std::collections::BTreeSet; @@ -29,11 +29,16 @@ use std::time::{SystemTime, UNIX_EPOCH}; use super::workspace_catalog::PlatformV2Mapping; mod custody; +mod delivery; mod notes; pub use custody::{ PlatformReviewCustodyStore, ReviewCustodyError, ReviewCustodyPresentation, ReviewCustodyRecovery, }; +pub use delivery::{ + advertised_agent_deliveries, review_delivery_control, AdvertisedReviewDelivery, + ReviewAgentDeliveryProjection, ReviewDeliveryWithheld, +}; pub use notes::{PlatformReviewNote, PlatformReviewNoteStore, ReviewNoteError}; mod worktree; @@ -169,39 +174,59 @@ impl PlatformReviewActionPreview { ) } + /// Prepare the unconfirmed batch delivery of advertised review comments. + /// + /// Every coordinate that crosses the network is taken verbatim from the + /// capability: the comment ids and their `expected_comment_revision` come + /// from `agent_deliverable_comments`, and the request is pinned to the + /// capability's own `snapshot_revision`. The snapshot is consulted only to + /// prove the two reads agree; it never supplies a value. + /// + /// This is the unconfirmed lane. It deliberately carries no confirmation + /// digest and no receipt correlation — see [`delivery`] for why both would + /// be wrong here, and why minting the second would actively break receipt + /// recovery. The advertisement is the fence, which is why an unadvertised + /// selection refuses here instead of being posted and refused remotely. pub fn batch_send_comments( target: PlatformReviewTarget, review: &PlatformReviewSemantic, + capabilities: &ReviewCapabilities, comment_ids: &[String], ) -> Result { validate_review_target(&target, review)?; - if review.review.freshness.state != ReviewFreshnessState::Fresh || comment_ids.is_empty() { - return Err("review comments are not actionable"); + if comment_ids.is_empty() { + return Err("review comment selection is empty"); } let selected = comment_ids.iter().collect::>(); if selected.len() != comment_ids.len() { return Err("review comment selection is duplicated"); } - let comments = comment_ids + let advertised = advertised_agent_deliveries(Some(capabilities), &target, review); + if advertised.is_empty() { + return Err("review agent delivery is not advertised"); + } + // Walk the advertisement rather than the selection. The server sorts + // the advertised list by comment id and the protocol requires a batch + // to be strictly ordered by that same id, so filtering here yields + // wire order for free; walking the selection would not. + let comments = advertised .iter() - .map(|id| { - let comment = review - .comments - .iter() - .find(|comment| &comment.id == id) - .filter(|comment| review.comment_is_batch_actionable(&comment.id)) - .ok_or("review comment is not actionable")?; + .filter(|entry| selected.contains(&entry.comment_id)) + .map(|entry| { Ok(ReviewCommentTarget::new( - ReviewCommentId::new(comment.id.clone()) + ReviewCommentId::new(entry.comment_id.clone()) .map_err(|_| "review comment identity is invalid")?, - comment.revision, + entry.expected_comment_revision, )) }) .collect::, &'static str>>()?; + if comments.len() != selected.len() { + return Err("review comment is not advertised as deliverable"); + } let nonce = review_nonce(); Self::review_action( target, - review.revision, + capabilities.snapshot_revision(), ReviewAction::BatchSendCommentsToAgent { comments }, &nonce, None, @@ -364,31 +389,77 @@ impl PlatformReviewActionPreview { self.confirmation.as_ref() } - /// Revalidate a confirmed rerun against the latest authoritative - /// capability generation before crossing the durable dispatch fence. + /// Whether this preview must be revalidated against the latest + /// authoritative capability generation before it crosses the durable + /// dispatch fence. + /// + /// A confirmed rerun must, because its digest names a plan the server + /// preflighted. A batch delivery must for the opposite reason: it carries + /// no digest at all, so the advertisement *is* its fence, and a list that + /// has since been retracted has to refuse locally rather than be posted. + #[must_use] + pub const fn requires_capability_revalidation(&self) -> bool { + self.confirmation.is_some() + || matches!(self.action, ReviewAction::BatchSendCommentsToAgent { .. }) + } + + /// Revalidate a confirmed rerun or an unconfirmed batch delivery against + /// the latest authoritative capability generation before crossing the + /// durable dispatch fence. #[must_use] pub fn matches_capabilities(&self, capabilities: &ReviewCapabilities) -> bool { - let ( - ReviewAction::RerunCheck { - check_id, - expected_check_revision, - }, - Some(confirmation), - ) = (&self.action, &self.confirmation) - else { - return self.confirmation.is_none(); - }; + match (&self.action, &self.confirmation) { + ( + ReviewAction::RerunCheck { + check_id, + expected_check_revision, + }, + Some(confirmation), + ) => { + self.capability_coordinates_match(capabilities) + && capabilities.workspace_revision() + == confirmation.expected_workspace_revision() + && capabilities.rerunnable_checks().iter().any(|capability| { + capability.check_id() == check_id + && capability.expected_check_revision() == *expected_check_revision + && capability.confirmation_digest() + == confirmation.confirmation_digest() + && capability.receipt_correlation_digest() + == confirmation.receipt_correlation_digest() + }) + } + // The unconfirmed delivery lane has no digest to compare, so the + // comparison is the advertisement itself: every comment must still + // be listed, at exactly the revision this preview committed to. + // + // That is what makes a reused list refuse here instead of reaching + // the server. Settling a delivery moves the comment out of + // `not_sent`/`refused` and bumps both the snapshot revision and the + // comment's own revision, so a re-read after settlement cannot + // still advertise it — whatever fresh idempotency key a caller + // attaches. + (ReviewAction::BatchSendCommentsToAgent { comments }, None) => { + self.capability_coordinates_match(capabilities) + && comments.iter().all(|comment| { + capabilities + .agent_deliverable_comments() + .iter() + .any(|capability| { + capability.comment_id() == comment.comment_id() + && capability.expected_comment_revision() + == comment.expected_revision() + }) + }) + } + (_, None) => true, + _ => false, + } + } + + fn capability_coordinates_match(&self, capabilities: &ReviewCapabilities) -> bool { capabilities.project() == &self.target.project && capabilities.workspace() == &self.target.workspace && capabilities.snapshot_revision() == self.expected_revision - && capabilities.workspace_revision() == confirmation.expected_workspace_revision() - && capabilities.rerunnable_checks().iter().any(|capability| { - capability.check_id() == check_id - && capability.expected_check_revision() == *expected_check_revision - && capability.confirmation_digest() == confirmation.confirmation_digest() - && capability.receipt_correlation_digest() - == confirmation.receipt_correlation_digest() - }) } #[must_use] @@ -547,9 +618,13 @@ pub enum PlatformReviewLoad { /// Authenticated server-advertised review mutation capabilities for one exact /// project/workspace snapshot. An unavailable capability load grants nothing. +/// +/// The available payload is boxed because `ReviewCapabilities` now carries the +/// agent-delivery list and three pull-request slots, which makes it far larger +/// than a refusal's two strings. #[derive(Clone, Debug, Eq, PartialEq)] pub enum PlatformReviewCapabilitiesLoad { - Available(ReviewCapabilities), + Available(Box), Unavailable(PlatformReviewUnavailable), } @@ -1081,6 +1156,110 @@ mod tests { const RENDER_CORPUS: &[u8] = include_bytes!("../../tests/fixtures/platform-v2-render-conformance-v1.json"); + /// Mint the advertisement a coherent server would return for `comment_ids` + /// at this review's exact snapshot revision. + /// + /// Every value comes from the review itself, so the helper cannot advertise + /// a comment revision the snapshot disagrees with — the tests that need + /// that divergence build it explicitly. + fn delivery_capabilities( + target: &PlatformReviewTarget, + review: &PlatformReviewSemantic, + comment_ids: &[&str], + ) -> ReviewCapabilities { + let authority = ReviewAuthority::new( + ReviewAuthorityKind::Review, + automonique_protocol::platform_v2_review::ReviewAuthorityId::new( + review.review.authority.id.clone(), + ) + .unwrap(), + ); + let advertised = comment_ids + .iter() + .map(|id| { + let comment = review.comments.iter().find(|c| c.id == *id).unwrap(); + ReviewAgentDeliveryCapability::new( + ReviewCommentId::new(comment.id.clone()).unwrap(), + comment.revision, + authority.clone(), + ) + .unwrap() + }) + .collect(); + ReviewCapabilities::new( + target.project.clone(), + target.workspace.clone(), + review.revision, + Revision::new(91).unwrap(), + Vec::new(), + advertised, + ReviewPullRequestCapabilities::default(), + ) + .unwrap() + } + + /// Mirror the server's own eligibility rule for `agent_deliverable_comments`. + /// + /// The daemon advertises a comment only when the caller holds the review + /// authority the snapshot names, the review is fresh, and the comment is + /// still in `not_sent` or `refused`. Reproducing that rule here — rather + /// than hand-writing each expected list — is what lets the delivery tests + /// *show* an advertisement retracting instead of asserting that it does. + fn advertise( + target: &PlatformReviewTarget, + review: &PlatformReviewSemantic, + ) -> ReviewCapabilities { + let eligible = if review.review.authority.kind == ReviewAuthorityKind::Review + && review.review.freshness.state == ReviewFreshnessState::Fresh + { + review + .comments + .iter() + .filter(|comment| { + matches!( + comment.agent_state, + CommentAgentState::NotSent | CommentAgentState::Refused + ) + }) + .map(|comment| comment.id.as_str()) + .collect::>() + } else { + Vec::new() + }; + delivery_capabilities(target, review, &eligible) + } + + /// Mirror the settlement the daemon performs when a delivery completes: + /// the delivered comment leaves the sendable states and bumps its own + /// revision, and the snapshot revision bumps with it. + fn settle(review: &PlatformReviewSemantic, delivered: &[&str]) -> PlatformReviewSemantic { + let mut settled = review.clone(); + settled.revision = Revision::new(review.revision.get() + 1).unwrap(); + for comment in &mut settled.comments { + if delivered.contains(&comment.id.as_str()) { + comment.agent_state = CommentAgentState::Sent; + comment.revision = Revision::new(comment.revision.get() + 1).unwrap(); + } + } + settled + } + + /// The canonical fixture carries one already-sent comment. Batch delivery + /// needs at least two sendable ones, and needs their snapshot order to + /// differ from their sorted order, so the wire ordering requirement is + /// exercised rather than satisfied by accident. + fn deliverable_review() -> PlatformReviewSemantic { + let snapshot = decode_review_snapshot(CANONICAL_FIXTURE).unwrap(); + let mut review = PlatformReviewSemantic::from(&snapshot); + review.comments[0].agent_state = CommentAgentState::NotSent; + let mut second = review.comments[0].clone(); + second.id = "comment-0".to_owned(); + second.revision = Revision::new(review.comments[0].revision.get() + 3).unwrap(); + second.agent_state = CommentAgentState::Refused; + review.comments.push(second); + review + } + fn rerun_capabilities( target: &PlatformReviewTarget, review: &PlatformReviewSemantic, @@ -1105,6 +1284,8 @@ mod tests { ReviewReceiptCorrelationDigest::new("b".repeat(64)).unwrap(), ) .unwrap()], + Vec::new(), + ReviewPullRequestCapabilities::default(), ) .unwrap() } @@ -1471,6 +1652,7 @@ mod tests { let batch = PlatformReviewActionPreview::batch_send_comments( target.clone(), &semantic, + &delivery_capabilities(&target, &semantic, &[&semantic.comments[0].id]), &[semantic.comments[0].id.clone()], ) .unwrap(); @@ -1626,9 +1808,13 @@ mod tests { wrong_authority.checks[0].authority.kind = ReviewAuthorityKind::Review; wrong_authority.pull_request.authority.kind = ReviewAuthorityKind::Review; assert!(!wrong_authority.approval_is_actionable()); + // The advertisement is minted from the review the snapshot names, so a + // Git review authority cannot produce one at all: the capability's own + // constructor refuses a non-review authority. assert!(PlatformReviewActionPreview::batch_send_comments( target.clone(), &wrong_authority, + &delivery_capabilities(&target, &semantic, &[&semantic.comments[0].id]), &[wrong_authority.comments[0].id.clone()], ) .is_err()); @@ -1674,8 +1860,9 @@ mod tests { ) .is_err()); assert!(PlatformReviewActionPreview::batch_send_comments( - target, + target.clone(), &semantic, + &delivery_capabilities(&target, &semantic, &[&semantic.comments[0].id]), &[ semantic.comments[0].id.clone(), semantic.comments[0].id.clone() @@ -1684,6 +1871,158 @@ mod tests { .is_err()); } + // SDTEST-1863 + #[test] + fn advertised_delivery_is_sent_verbatim_and_a_reused_advertisement_is_refused() { + let review = deliverable_review(); + let snapshot = decode_review_snapshot(CANONICAL_FIXTURE).unwrap(); + let target = PlatformReviewTarget { + project: ProjectId::new("project-1").unwrap(), + workspace: snapshot.workspace().clone(), + }; + + // The server advertises both sendable comments, sorted by id, which is + // the opposite of their order inside the snapshot. + let advertised = advertise(&target, &review); + let ids = advertised + .agent_deliverable_comments() + .iter() + .map(|capability| capability.comment_id().as_str()) + .collect::>(); + assert_eq!(ids, vec!["comment-0", "comment-1"]); + assert_ne!( + ids, + review + .comments + .iter() + .map(|comment| comment.id.as_str()) + .collect::>() + ); + + let preview = PlatformReviewActionPreview::batch_send_comments( + target.clone(), + &review, + &advertised, + &["comment-1".to_owned(), "comment-0".to_owned()], + ) + .unwrap(); + // Every coordinate is the capability's, and the request is pinned to + // the capability's snapshot revision. + assert_eq!(preview.expected_revision(), advertised.snapshot_revision()); + assert!(preview.confirmation().is_none()); + let ReviewAction::BatchSendCommentsToAgent { comments } = preview.action() else { + panic!("batch preview changed action family"); + }; + assert_eq!( + comments + .iter() + .map(|comment| (comment.comment_id().as_str(), comment.expected_revision())) + .collect::>(), + advertised + .agent_deliverable_comments() + .iter() + .map(|capability| ( + capability.comment_id().as_str(), + capability.expected_comment_revision() + )) + .collect::>() + ); + assert!(preview.requires_capability_revalidation()); + assert!(preview.matches_capabilities(&advertised)); + + // A capability response minted against another snapshot revision must + // construct nothing, or the preview would pin itself to a revision the + // active review never observed. + let superseded = advertise(&target, &settle(&review, &[])); + assert_ne!(superseded.snapshot_revision(), review.revision); + assert!(PlatformReviewActionPreview::batch_send_comments( + target.clone(), + &review, + &superseded, + &["comment-1".to_owned()], + ) + .is_err()); + // A comment that exists and is sendable, but was not advertised, is + // refused locally rather than posted for the server to refuse. + let partial = delivery_capabilities(&target, &review, &["comment-0"]); + assert!(PlatformReviewActionPreview::batch_send_comments( + target.clone(), + &review, + &partial, + &["comment-0".to_owned(), "comment-1".to_owned()], + ) + .is_err()); + + // One delivery settles. Re-read the advertisement rather than reusing + // the list: the delivered comment left `refused`, its own revision + // bumped, and the snapshot revision bumped with it. + let settled = settle(&review, &["comment-0"]); + let readvertised = advertise(&target, &settled); + assert_eq!( + readvertised + .agent_deliverable_comments() + .iter() + .map(|capability| capability.comment_id().as_str()) + .collect::>(), + vec!["comment-1"], + "settling one delivery must retract only that comment" + ); + assert_eq!(readvertised.snapshot_revision(), settled.revision); + + // The preview built from the retracted list is now refused locally, so + // it never reaches the server. + assert!(!preview.matches_capabilities(&readvertised)); + + // And a fresh preview built from that same stale list — the exact + // request a duplicate-delivery bug would have to make: same batch, + // same advertised revisions, a brand new idempotency key — is refused + // for the same reason, which is why no digest is needed here. + let replay = PlatformReviewActionPreview::batch_send_comments( + target.clone(), + &review, + &advertised, + &["comment-1".to_owned(), "comment-0".to_owned()], + ) + .unwrap(); + assert_ne!(replay.idempotency_key(), preview.idempotency_key()); + assert!(!replay.matches_capabilities(&readvertised)); + + // Preparing against the settled snapshot cannot reconstruct it either: + // the delivered comment is no longer advertised at all. + assert!(PlatformReviewActionPreview::batch_send_comments( + target.clone(), + &settled, + &readvertised, + &["comment-0".to_owned()], + ) + .is_err()); + + // The survivor is still deliverable, at the new snapshot revision. + let next = PlatformReviewActionPreview::batch_send_comments( + target.clone(), + &settled, + &readvertised, + &["comment-1".to_owned()], + ) + .unwrap(); + assert_eq!(next.expected_revision(), settled.revision); + assert!(next.matches_capabilities(&readvertised)); + + // Settling the last one empties the advertisement entirely, and an + // empty advertisement can construct nothing. + let drained = settle(&settled, &["comment-1"]); + let empty = advertise(&target, &drained); + assert!(empty.agent_deliverable_comments().is_empty()); + assert!(!next.matches_capabilities(&empty)); + assert!(PlatformReviewActionPreview::batch_send_comments( + target, + &drained, + &empty, + &["comment-1".to_owned()], + ) + .is_err()); + } + // SDTEST-1778 #[test] fn canonical_render_corpus_projects_every_cross_client_semantic_key() { diff --git a/crates/shelldeck-core/src/config/platform_review/custody.rs b/crates/shelldeck-core/src/config/platform_review/custody.rs index 83fa0454..8f94428e 100644 --- a/crates/shelldeck-core/src/config/platform_review/custody.rs +++ b/crates/shelldeck-core/src/config/platform_review/custody.rs @@ -9,7 +9,7 @@ use std::path::{Path, PathBuf}; use automonique_protocol::platform::ReceiptId; use automonique_protocol::platform_v2_review::{ - ReviewActionId, ReviewActorId, ReviewFileId, ReviewHunkId, + ReviewActionId, ReviewActorId, ReviewFileId, ReviewHunkId, MAX_REVIEW_COMMENTS, }; use serde::{Deserialize, Serialize}; use thiserror::Error; @@ -348,6 +348,25 @@ enum ReviewActionDisk { check_id: String, expected_check_revision: u64, }, + /// The unconfirmed batch delivery lane. + /// + /// It carries no `confirmation` — the server advertises no digest for it — + /// but it is still an exposed mutation, so it takes the same durable + /// at-most-once record as every other one. Without it a restart between + /// the POST and the receipt would leave a delivered batch indistinguishable + /// from one that never left, and re-preparing would mint a fresh + /// idempotency key. + BatchSendCommentsToAgent { + comments: Vec, + }, +} + +/// One comment target inside a persisted batch delivery. +#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +struct ReviewCommentTargetDisk { + comment_id: String, + expected_revision: u64, } impl ReviewActionDisk { @@ -377,6 +396,15 @@ impl ReviewActionDisk { check_id: check_id.as_str().to_owned(), expected_check_revision: expected_check_revision.get(), }, + ReviewAction::BatchSendCommentsToAgent { comments } => Self::BatchSendCommentsToAgent { + comments: comments + .iter() + .map(|comment| ReviewCommentTargetDisk { + comment_id: comment.comment_id().as_str().to_owned(), + expected_revision: comment.expected_revision().get(), + }) + .collect(), + }, _ => return Err(ReviewCustodyError::UnsupportedAction), }) } @@ -420,6 +448,27 @@ impl ReviewActionDisk { expected_check_revision: Revision::new(*expected_check_revision) .map_err(|_| ReviewCustodyError::DocumentInvalid)?, }, + Self::BatchSendCommentsToAgent { comments } => { + // Bound before allocating. `validate_client_shape` would also + // reject an oversized batch, but only after building the whole + // vector out of a document this process has not yet trusted. + if comments.len() > MAX_REVIEW_COMMENTS { + return Err(ReviewCustodyError::DocumentInvalid); + } + ReviewAction::BatchSendCommentsToAgent { + comments: comments + .iter() + .map(|comment| { + Ok(ReviewCommentTarget::new( + ReviewCommentId::new(comment.comment_id.clone()) + .map_err(|_| ReviewCustodyError::DocumentInvalid)?, + Revision::new(comment.expected_revision) + .map_err(|_| ReviewCustodyError::DocumentInvalid)?, + )) + }) + .collect::, ReviewCustodyError>>()?, + } + } }) } } @@ -955,6 +1004,105 @@ mod tests { ); } + /// The advertisement a coherent server would mint for this snapshot's one + /// sendable comment, and the preview built verbatim from it. + fn batch_delivery() -> PlatformReviewActionPreview { + let mut review = semantic(); + review.comments[0].agent_state = CommentAgentState::NotSent; + let target = target(&review); + let capabilities = ReviewCapabilities::new( + target.project.clone(), + target.workspace.clone(), + review.revision, + Revision::new(91).unwrap(), + Vec::new(), + vec![ReviewAgentDeliveryCapability::new( + ReviewCommentId::new(review.comments[0].id.clone()).unwrap(), + review.comments[0].revision, + ReviewAuthority::new( + ReviewAuthorityKind::Review, + automonique_protocol::platform_v2_review::ReviewAuthorityId::new( + review.review.authority.id.clone(), + ) + .unwrap(), + ), + ) + .unwrap()], + ReviewPullRequestCapabilities::default(), + ) + .unwrap(); + PlatformReviewActionPreview::batch_send_comments( + target, + &review, + &capabilities, + &[review.comments[0].id.clone()], + ) + .unwrap() + } + + // SDTEST-1865 — the unconfirmed batch delivery takes the same durable + // at-most-once lane as every other exposed mutation, and survives the + // round trip through disk with its exact comment set intact. + #[test] + fn unconfirmed_batch_delivery_is_fenced_and_survives_the_disk_round_trip() { + let directory = TempRoot::new(); + let preview = batch_delivery(); + // No confirmation digest: this lane is fenced by the advertisement and + // the domain state machine, never by a digest. + assert!(preview.confirmation().is_none()); + // It must still be revalidated before dispatch, exactly like a + // confirmed rerun and unlike a comment or an approval. + assert!(preview.requires_capability_revalidation()); + + // Before this slice the disk format knew only AddComment, + // ApproveReview and RerunCheck, so a batch delivery could not be + // recorded at all and would have refused at the fence. + let store = PlatformReviewCustodyStore::open(store_path(&directory)).unwrap(); + store.prepare(&preview).unwrap(); + assert!(matches!( + store.prepare(&batch_delivery()), + Err(ReviewCustodyError::EffectAlreadyPending) + )); + + // A restart before dispatch reports the preview as never started, and + // the exact action survives serialization: same comments, same + // revisions, same order. + let restarted = PlatformReviewCustodyStore::open(store_path(&directory)).unwrap(); + assert_eq!( + restarted.recovery(preview.target()).unwrap(), + Some(ReviewCustodyRecovery::NeverStarted(preview.clone())) + ); + + restarted.mark_dispatched(&preview).unwrap(); + assert_eq!( + PlatformReviewCustodyStore::open(store_path(&directory)) + .unwrap() + .recovery(preview.target()) + .unwrap(), + Some(ReviewCustodyRecovery::LookupOnly(preview.clone())), + "a restart mid-delivery must look the receipt up, never re-post" + ); + + let reopened = PlatformReviewCustodyStore::open(store_path(&directory)).unwrap(); + reopened + .record_receipt( + &preview, + &receipt(&preview, ReviewReceiptOutcome::Completed, "reviewer-9"), + ) + .unwrap(); + assert_eq!( + PlatformReviewCustodyStore::open(store_path(&directory)) + .unwrap() + .recovery(preview.target()) + .unwrap(), + Some(ReviewCustodyRecovery::Terminal(ReviewCustodyPresentation { + outcome: "completed".to_owned(), + detail: "action-1 · final".to_owned(), + actor: Some("reviewer-9".to_owned()), + })) + ); + } + #[cfg(unix)] // SDTEST-1826 — linked/replaced custody paths and lock files are never // followed outside ShellDeck's private directory boundary. diff --git a/crates/shelldeck-core/src/config/platform_review/delivery.rs b/crates/shelldeck-core/src/config/platform_review/delivery.rs new file mode 100644 index 00000000..655f8def --- /dev/null +++ b/crates/shelldeck-core/src/config/platform_review/delivery.rs @@ -0,0 +1,504 @@ +//! Server-advertised batch delivery of review comments to the authorized session. +//! +//! This module is a read-side projection, exactly like [`super::worktree`]. It +//! grants nothing: it reports which comments the server has *proven* it can +//! deliver to the retained agent session bound to this review coordinate, and +//! otherwise reports which fence is missing so the surface can say so instead +//! of offering a control that would refuse. +//! +//! # Why this lane carries no confirmation digest +//! +//! A confirmed check rerun needs one because the client names the target and +//! the effect fires in a system the daemon does not own, so only a digest can +//! bind the executed write to the preflighted plan. Delivery to a retained +//! session differs on every axis that motivated the digest: +//! +//! * The target is never on the wire. Provider and session identity come from +//! the operator-owned registry, keyed by the review coordinate, so a client +//! can neither name nor redirect the session it reaches. +//! * The snapshot revision is fenced twice server-side, by the store's +//! current-revision check and again by `resolve_action`. +//! * The comment set is fenced by the batch arm of `resolve_action`, which +//! requires every comment to exist at exactly the advertised revision and to +//! still be in `not_sent` or `refused`. +//! +//! Exactly-once therefore falls out of the domain state machine rather than a +//! digest: settling a delivery moves the comment out of those two states and +//! bumps both the snapshot revision and the comment's own revision, so the +//! advertisement that authorized a send cannot construct a second one. +//! +//! Minting a receipt correlation here would be worse than redundant: the +//! host's receipt lookup *skips* a retained action whenever the request +//! carries a correlation digest, so advertising one would hand this client a +//! token that makes its own receipt unfindable. A fence that breaks recovery +//! is not a fence. +//! +//! The client consequence is the one rule this module exists to enforce: the +//! advertisement is the fence, so it must be re-read after every settlement +//! rather than reused. See [`super::PlatformReviewActionPreview::matches_capabilities`]. + +use super::{ + PlatformReviewSemantic, PlatformReviewTarget, ReviewAuthorityKind, ReviewCapabilities, +}; +use automonique_protocol::primitives::Revision; + +/// Why the projection offers no batch send-to-agent control. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum ReviewDeliveryWithheld { + /// No capability response is attributed to this exact project, workspace + /// and snapshot revision, so nothing has been advertised at all. + NoServerCapability, + /// The capability response is exact and its advertised list is empty. + /// That is the server's honest fail-closed answer — no registry binding, + /// no reachable delivery adapter, a stale review, or no comment currently + /// in a sendable state — and it must produce no control. + NoDeliverableComment, + /// No durable at-most-once custody store is available in this process. + NoCustodyLane, +} + +impl ReviewDeliveryWithheld { + /// Every reason a surface must be able to explain. + pub const ALL: [Self; 3] = [ + Self::NoServerCapability, + Self::NoDeliverableComment, + Self::NoCustodyLane, + ]; + + #[must_use] + pub const fn as_str(self) -> &'static str { + match self { + Self::NoServerCapability => "no_server_capability", + Self::NoDeliverableComment => "no_deliverable_comment", + Self::NoCustodyLane => "no_custody_lane", + } + } + + /// Canonical cross-client presentation key, carrying no localized copy. + #[must_use] + pub fn semantic_key(self) -> String { + format!("delivery_withheld.{}", self.as_str()) + } +} + +/// One comment the server advertised as deliverable, carried verbatim. +/// +/// `expected_comment_revision` is the server's value, never the snapshot's. +/// The two agree on a coherent read — that is what [`advertised_agent_deliveries`] +/// checks — but the value that crosses the network is this one, because it is +/// the one the server committed to accepting. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct AdvertisedReviewDelivery { + pub comment_id: String, + pub expected_comment_revision: Revision, + pub authority_id: String, +} + +/// The exact advertised delivery set for one active snapshot. +/// +/// Every entry survives three checks: the capability response is attributed to +/// this exact project, workspace and snapshot revision; its authority is the +/// review authority the snapshot itself names; and the comment is present in +/// that same snapshot, at the advertised revision, in a sendable state. +/// +/// The third check is a torn-read guard, not a second source of authority. The +/// capability list and the snapshot are two separate reads, and a coherent +/// server mints them from one projection, so a disagreement means they +/// straddled a change. Refusing the entry is the fail-closed answer; it can +/// never *add* an entry the server did not advertise. +#[must_use] +pub fn advertised_agent_deliveries( + capabilities: Option<&ReviewCapabilities>, + target: &PlatformReviewTarget, + review: &PlatformReviewSemantic, +) -> Vec { + let Some(capabilities) = capabilities else { + return Vec::new(); + }; + if capabilities.project() != &target.project + || capabilities.workspace() != &target.workspace + || capabilities.snapshot_revision() != review.revision + || review.workspace_kind != target.workspace.kind() + || review.workspace_id != target.workspace.id() + { + return Vec::new(); + } + capabilities + .agent_deliverable_comments() + .iter() + .filter(|capability| { + capability.authority().kind() == ReviewAuthorityKind::Review + && capability.authority().id().as_str() == review.review.authority.id + && review.review.authority.kind == ReviewAuthorityKind::Review + }) + .filter(|capability| { + review.comments.iter().any(|comment| { + comment.id == capability.comment_id().as_str() + && comment.revision == capability.expected_comment_revision() + }) && review.comment_is_batch_actionable(capability.comment_id().as_str()) + }) + .map(|capability| AdvertisedReviewDelivery { + comment_id: capability.comment_id().as_str().to_owned(), + expected_comment_revision: capability.expected_comment_revision(), + authority_id: capability.authority().id().as_str().to_owned(), + }) + .collect() +} + +/// Decide whether a batch send-to-agent control may be rendered at all. +/// +/// Both fences must hold, exactly like the confirmed check rerun: the server +/// must have advertised at least one deliverable comment for this exact +/// snapshot, and the durable custody store must be able to record the +/// at-most-once boundary before dispatch. This lane needs no *confirmation +/// digest*, but it still needs the durable record every other exposed mutation +/// gets: a restart between the POST and the receipt must not make a delivered +/// batch indistinguishable from one that never left. +/// +/// A missing fence makes the control absent, never optimistically disabled. +/// +/// # Errors +/// +/// Returns the first unmet fence. +pub const fn review_delivery_control( + exact_capability: bool, + advertised: bool, + custody_available: bool, +) -> Result<(), ReviewDeliveryWithheld> { + if !exact_capability { + Err(ReviewDeliveryWithheld::NoServerCapability) + } else if !advertised { + Err(ReviewDeliveryWithheld::NoDeliverableComment) + } else if !custody_available { + Err(ReviewDeliveryWithheld::NoCustodyLane) + } else { + Ok(()) + } +} + +/// Everything a surface needs to render the delivery lane for one snapshot. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct ReviewAgentDeliveryProjection { + advertised: Vec, + /// Why no control may be offered, when none may. + pub control: Result<(), ReviewDeliveryWithheld>, +} + +impl ReviewAgentDeliveryProjection { + #[must_use] + pub fn new( + review: &PlatformReviewSemantic, + target: &PlatformReviewTarget, + capabilities: Option<&ReviewCapabilities>, + custody_available: bool, + ) -> Self { + // "Exact" is about attribution, not content: a capability response for + // this project, workspace and snapshot revision was received. An empty + // advertised list inside it is a different, more specific answer, and + // the surface must be able to say which one it got. + let exact_capability = capabilities.is_some_and(|capabilities| { + capabilities.project() == &target.project + && capabilities.workspace() == &target.workspace + && capabilities.snapshot_revision() == review.revision + }); + let advertised = advertised_agent_deliveries(capabilities, target, review); + let control = + review_delivery_control(exact_capability, !advertised.is_empty(), custody_available); + Self { + advertised, + control, + } + } + + /// The projection for a surface that has no exactly attributed target. + /// + /// Nothing was advertised because nothing could have been, so this is the + /// same fail-closed answer as an absent capability response. + #[must_use] + pub const fn unavailable() -> Self { + Self { + advertised: Vec::new(), + control: Err(ReviewDeliveryWithheld::NoServerCapability), + } + } + + /// The advertised set, in the capability's own order. + /// + /// The server sorts it by comment id, and the protocol requires a batch to + /// be strictly ordered by that same id, so a selection filtered out of + /// this slice is already in wire order. + #[must_use] + pub fn advertised(&self) -> &[AdvertisedReviewDelivery] { + &self.advertised + } + + /// Whether this exact comment may carry a send control. + #[must_use] + pub fn advertises(&self, comment_id: &str) -> bool { + self.advertised + .iter() + .any(|entry| entry.comment_id == comment_id) + } +} + +#[cfg(test)] +mod tests { + use super::super::{ + CommentAgentState, PlatformReviewSemantic, ReviewAgentDeliveryCapability, ReviewAuthority, + ReviewCommentId, ReviewFreshnessState, ReviewPullRequestCapabilities, + }; + use super::*; + use automonique_protocol::platform_v2::{ProjectId, WorkContextIdentity}; + use automonique_protocol::platform_v2_review::ReviewAuthorityId; + use automonique_protocol::platform_v2_review_api::decode_review_snapshot; + + const CANONICAL_FIXTURE: &[u8] = + include_bytes!("../../../tests/fixtures/platform-v2-review-v2.json"); + + fn fixture() -> (PlatformReviewTarget, PlatformReviewSemantic) { + let snapshot = decode_review_snapshot(CANONICAL_FIXTURE).unwrap(); + let mut review = PlatformReviewSemantic::from(&snapshot); + review.comments[0].agent_state = CommentAgentState::NotSent; + let target = PlatformReviewTarget { + project: ProjectId::new("project-1").unwrap(), + workspace: snapshot.workspace().clone(), + }; + (target, review) + } + + /// Build a capability response with arbitrary coordinates, so each fence + /// can be broken one at a time. + fn capabilities( + project: &ProjectId, + workspace: &WorkContextIdentity, + snapshot_revision: Revision, + entries: Vec<(&str, Revision, &str)>, + ) -> ReviewCapabilities { + let advertised = entries + .into_iter() + .map(|(id, revision, authority)| { + ReviewAgentDeliveryCapability::new( + ReviewCommentId::new(id.to_owned()).unwrap(), + revision, + ReviewAuthority::new( + ReviewAuthorityKind::Review, + ReviewAuthorityId::new(authority.to_owned()).unwrap(), + ), + ) + .unwrap() + }) + .collect(); + ReviewCapabilities::new( + project.clone(), + workspace.clone(), + snapshot_revision, + Revision::new(91).unwrap(), + Vec::new(), + advertised, + ReviewPullRequestCapabilities::default(), + ) + .unwrap() + } + + fn exact(target: &PlatformReviewTarget, review: &PlatformReviewSemantic) -> ReviewCapabilities { + let comment = &review.comments[0]; + capabilities( + &target.project, + &target.workspace, + review.revision, + vec![( + comment.id.as_str(), + comment.revision, + review.review.authority.id.as_str(), + )], + ) + } + + // SDTEST-1864 + #[test] + fn every_delivery_fence_fails_closed_with_its_own_distinct_reason() { + let (target, review) = fixture(); + let exact_capabilities = exact(&target, &review); + + let granted = + ReviewAgentDeliveryProjection::new(&review, &target, Some(&exact_capabilities), true); + assert_eq!(granted.control, Ok(())); + assert!(granted.advertises(&review.comments[0].id)); + assert_eq!( + granted.advertised(), + &[AdvertisedReviewDelivery { + comment_id: review.comments[0].id.clone(), + expected_comment_revision: review.comments[0].revision, + authority_id: review.review.authority.id.clone(), + }] + ); + + // No capability response at all. + assert_eq!( + ReviewAgentDeliveryProjection::new(&review, &target, None, true).control, + Err(ReviewDeliveryWithheld::NoServerCapability) + ); + assert_eq!( + ReviewAgentDeliveryProjection::unavailable().control, + Err(ReviewDeliveryWithheld::NoServerCapability) + ); + + // A response for another project, another workspace, or another + // snapshot revision is not this snapshot's answer. + let foreign_project = capabilities( + &ProjectId::new("project-2").unwrap(), + &target.workspace, + review.revision, + vec![( + review.comments[0].id.as_str(), + review.comments[0].revision, + review.review.authority.id.as_str(), + )], + ); + assert_eq!( + ReviewAgentDeliveryProjection::new(&review, &target, Some(&foreign_project), true) + .control, + Err(ReviewDeliveryWithheld::NoServerCapability) + ); + let stale_snapshot = capabilities( + &target.project, + &target.workspace, + Revision::new(review.revision.get() + 1).unwrap(), + vec![( + review.comments[0].id.as_str(), + review.comments[0].revision, + review.review.authority.id.as_str(), + )], + ); + assert_eq!( + ReviewAgentDeliveryProjection::new(&review, &target, Some(&stale_snapshot), true) + .control, + Err(ReviewDeliveryWithheld::NoServerCapability) + ); + + // `advertised_agent_deliveries` is asserted directly, not only through + // the projection: the action preview calls it on its own, so a fence + // that lived only in `ReviewAgentDeliveryProjection::new` would let a + // preview pin itself to a foreign or superseded snapshot revision. + assert!( + advertised_agent_deliveries(Some(&exact_capabilities), &target, &review).len() == 1 + ); + for hole in [&foreign_project, &stale_snapshot] { + assert!( + advertised_agent_deliveries(Some(hole), &target, &review).is_empty(), + "a capability response for another coordinate advertises nothing" + ); + } + assert!(advertised_agent_deliveries(None, &target, &review).is_empty()); + let foreign_workspace = PlatformReviewTarget { + project: target.project.clone(), + workspace: WorkContextIdentity::parse_local(target.workspace.kind(), "wc_user_foreign") + .unwrap(), + }; + assert!(advertised_agent_deliveries( + Some(&exact_capabilities), + &foreign_workspace, + &review + ) + .is_empty()); + + // An exact response advertising nothing is the server's honest + // fail-closed answer, and must read differently from an absent one. + let empty = capabilities( + &target.project, + &target.workspace, + review.revision, + Vec::new(), + ); + assert_eq!( + ReviewAgentDeliveryProjection::new(&review, &target, Some(&empty), true).control, + Err(ReviewDeliveryWithheld::NoDeliverableComment) + ); + + // Custody is still required. This lane needs no confirmation digest, + // but it is an exposed mutation and takes the same durable record. + assert_eq!( + ReviewAgentDeliveryProjection::new(&review, &target, Some(&exact_capabilities), false) + .control, + Err(ReviewDeliveryWithheld::NoCustodyLane) + ); + + // Torn reads. The capability list and the snapshot are two separate + // reads; a coherent server mints them from one projection, so any + // disagreement means they straddled a change and the entry is dropped. + let wrong_comment_revision = capabilities( + &target.project, + &target.workspace, + review.revision, + vec![( + review.comments[0].id.as_str(), + Revision::new(review.comments[0].revision.get() + 1).unwrap(), + review.review.authority.id.as_str(), + )], + ); + assert_eq!( + ReviewAgentDeliveryProjection::new( + &review, + &target, + Some(&wrong_comment_revision), + true + ) + .control, + Err(ReviewDeliveryWithheld::NoDeliverableComment) + ); + let unknown_comment = capabilities( + &target.project, + &target.workspace, + review.revision, + vec![( + "comment-absent", + review.comments[0].revision, + review.review.authority.id.as_str(), + )], + ); + assert_eq!( + ReviewAgentDeliveryProjection::new(&review, &target, Some(&unknown_comment), true) + .control, + Err(ReviewDeliveryWithheld::NoDeliverableComment) + ); + let foreign_authority = capabilities( + &target.project, + &target.workspace, + review.revision, + vec![( + review.comments[0].id.as_str(), + review.comments[0].revision, + "authority-other", + )], + ); + assert_eq!( + ReviewAgentDeliveryProjection::new(&review, &target, Some(&foreign_authority), true) + .control, + Err(ReviewDeliveryWithheld::NoDeliverableComment) + ); + + // A comment already delivered, and a review that is no longer fresh, + // are both unsendable however the advertisement reads. + let mut sent = review.clone(); + sent.comments[0].agent_state = CommentAgentState::Sent; + assert_eq!( + ReviewAgentDeliveryProjection::new(&sent, &target, Some(&exact_capabilities), true) + .control, + Err(ReviewDeliveryWithheld::NoDeliverableComment) + ); + let mut stale = review.clone(); + stale.review.freshness.state = ReviewFreshnessState::Stale; + assert_eq!( + ReviewAgentDeliveryProjection::new(&stale, &target, Some(&exact_capabilities), true) + .control, + Err(ReviewDeliveryWithheld::NoDeliverableComment) + ); + + // Every reason a surface must explain carries its own key. + let keys = ReviewDeliveryWithheld::ALL + .iter() + .map(|reason| reason.semantic_key()) + .collect::>(); + assert_eq!(keys.len(), ReviewDeliveryWithheld::ALL.len()); + } +} diff --git a/crates/shelldeck-ui/src/fleet_view.rs b/crates/shelldeck-ui/src/fleet_view.rs index ecb74cf6..84a5c563 100644 --- a/crates/shelldeck-ui/src/fleet_view.rs +++ b/crates/shelldeck-ui/src/fleet_view.rs @@ -25,10 +25,11 @@ use shelldeck_core::config::platform_review::{ ConflictResolution, PlatformReviewActionPreview, PlatformReviewCapabilitiesLoad, PlatformReviewConfirmationCoordinates, PlatformReviewCustodyStore, PlatformReviewLoad, PlatformReviewNote, PlatformReviewNoteStore, PlatformReviewRenderSemantic, - PlatformReviewSemantic, PlatformReviewTarget, ReviewAction, ReviewAnchorSemantic, - ReviewCustodyRecovery, ReviewPreviewWithheld, ReviewProposalKind, ReviewReceiptOutcome, - ReviewSafePreview, ReviewSafeText, ReviewStagingWithheld, ReviewWorktreeFile, - ReviewWorktreeLane, ReviewWorktreeLaneGroup, ReviewWorktreeProjection, + PlatformReviewSemantic, PlatformReviewTarget, ReviewAction, ReviewAgentDeliveryProjection, + ReviewAnchorSemantic, ReviewCustodyRecovery, ReviewDeliveryWithheld, ReviewPreviewWithheld, + ReviewProposalKind, ReviewReceiptOutcome, ReviewSafePreview, ReviewSafeText, + ReviewStagingWithheld, ReviewWorktreeFile, ReviewWorktreeLane, ReviewWorktreeLaneGroup, + ReviewWorktreeProjection, }; use crate::icons::lucide_icon; @@ -225,6 +226,18 @@ const fn review_staging_withheld_key(reason: ReviewStagingWithheld) -> &'static } } +const fn review_delivery_withheld_key(reason: ReviewDeliveryWithheld) -> &'static str { + match reason { + ReviewDeliveryWithheld::NoServerCapability => { + "fleet.review.delivery_withheld_no_server_capability" + } + ReviewDeliveryWithheld::NoDeliverableComment => { + "fleet.review.delivery_withheld_no_deliverable_comment" + } + ReviewDeliveryWithheld::NoCustodyLane => "fleet.review.delivery_withheld_no_custody_lane", + } +} + fn localized_review_proposal_kind(kind: ReviewProposalKind) -> String { match kind { ReviewProposalKind::Stage => t!("fleet.review.action_stage").to_string(), @@ -460,17 +473,30 @@ impl FleetView { if !selected_is_current { self.selected_review_anchor = None; } - match &review { - Some(PlatformReviewLoad::Available(review)) => { + // The selection is retained against the *advertisement*, never against + // the snapshot alone. A settled delivery moves its comments out of the + // sendable states and bumps the revision, so the freshly re-read + // capability no longer lists them and the selection empties itself — + // which is what stops the previous list from being reused. + match (&review, &target) { + (Some(PlatformReviewLoad::Available(review)), Some(target)) => { + let delivery = ReviewAgentDeliveryProjection::new( + review, + target, + capabilities + .as_ref() + .and_then(PlatformReviewCapabilitiesLoad::available), + self.review_custody.is_some(), + ); self.selected_review_comments - .retain(|id| review.comment_is_batch_actionable(id)); + .retain(|id| delivery.advertises(id)); } _ => self.selected_review_comments.clear(), } let preview_is_stale = self.pending_review_preview.as_ref().is_some_and(|preview| { target.as_ref() != Some(preview.target()) || !matches!(&review, Some(PlatformReviewLoad::Available(value)) if value.revision == preview.expected_revision()) - || (preview.confirmation().is_some() + || (preview.requires_capability_revalidation() && !capabilities .as_ref() .and_then(PlatformReviewCapabilitiesLoad::available) @@ -828,6 +854,62 @@ impl FleetView { self.admit_review_preview(preview) } + /// Toggle one advertised comment in the batch-delivery selection. + /// + /// A comment the current advertisement does not list can never enter the + /// selection, so a control that raced a capability refresh selects nothing + /// rather than staging an unsendable id. + fn set_review_comment_selected(&mut self, id: &str, selected: bool) { + if !selected { + self.selected_review_comments.remove(id); + return; + } + if self + .review_delivery() + .is_some_and(|delivery| delivery.advertises(id)) + { + self.selected_review_comments.insert(id.to_owned()); + } + } + + /// The advertised delivery projection for the exact active snapshot. + fn review_delivery(&self) -> Option { + let review = self.available_review().ok()?; + let target = self.review_target.as_ref()?; + Some(ReviewAgentDeliveryProjection::new( + review, + target, + self.available_review_capabilities().ok(), + self.review_custody.is_some(), + )) + } + + /// Prepare the unconfirmed batch delivery of the selected comments. + /// + /// The preview takes every coordinate from the capability, so this refuses + /// locally whenever the advertisement no longer covers the selection. + fn prepare_review_batch_send(&mut self) -> Result<(), ReviewPrepareRefusal> { + let target = self + .review_target + .clone() + .ok_or(ReviewPrepareRefusal::Preview)?; + let selected = self + .selected_review_comments + .iter() + .cloned() + .collect::>(); + let preview = PlatformReviewActionPreview::batch_send_comments( + target, + self.available_review() + .map_err(|_| ReviewPrepareRefusal::Preview)?, + self.available_review_capabilities() + .map_err(|_| ReviewPrepareRefusal::Preview)?, + &selected, + ) + .map_err(|_| ReviewPrepareRefusal::Preview)?; + self.admit_review_preview(preview) + } + fn prepare_review_approval(&mut self) -> Result<(), ReviewPrepareRefusal> { let target = self .review_target @@ -905,12 +987,17 @@ impl FleetView { return None; } let preview = self.pending_review_preview.as_ref()?; - if preview.confirmation().is_some() { + if preview.requires_capability_revalidation() { + let stale_detail = if preview.confirmation().is_some() { + "fleet.review.capability_stale" + } else { + "fleet.review.delivery_capability_stale" + }; let capabilities = self.available_review_capabilities().ok()?; if !preview.matches_capabilities(capabilities) { self.review_receipt = Some(( t!("fleet.review.state_refused").to_string(), - t!("fleet.review.capability_stale").to_string(), + t!(stale_detail).to_string(), )); return None; } @@ -1678,32 +1765,94 @@ impl FleetView { ); } + // Batch delivery to the authorized session. The send control exists + // only for the comments the server advertised in + // `agent_deliverable_comments` for this exact snapshot: the + // advertisement is the whole fence for this lane, because it carries + // no confirmation digest by design. + let delivery = self + .review_delivery() + .unwrap_or_else(ReviewAgentDeliveryProjection::unavailable); let mut comments = div().flex().flex_col().gap(px(4.0)); for comment in &review.comments { - comments = comments.child( - div() - .flex() - .items_center() - .gap(px(5.0)) - .min_w(px(0.0)) - .overflow_hidden() - .text_size(px(11.0)) - .text_color(ShellDeckColors::text_muted()) - .child( - div().flex_1().min_w(px(0.0)).child( - t!( - "fleet.review.comment", - actor = comment.actor.as_str(), - file = comment.anchor.file_id.as_str(), - side = comment.anchor.side.as_str(), - line = comment.anchor.line, - revision = comment.revision.get(), - body = comment.body.as_str() - ) - .to_string(), - ), + let deliverable = delivery.advertises(&comment.id); + let selected = self.selected_review_comments.contains(&comment.id); + let mut row = div() + .flex() + .items_center() + .gap(px(5.0)) + .min_w(px(0.0)) + .overflow_hidden() + .text_size(px(11.0)) + .text_color(ShellDeckColors::text_muted()) + .child( + div().flex_1().min_w(px(0.0)).child( + t!( + "fleet.review.comment", + actor = comment.actor.as_str(), + file = comment.anchor.file_id.as_str(), + side = comment.anchor.side.as_str(), + line = comment.anchor.line, + revision = comment.revision.get(), + body = comment.body.as_str() + ) + .to_string(), ), - ); + ); + if deliverable { + let select_entity = entity.clone(); + let select_id = comment.id.clone(); + // Same widget, same variants and same copy as the note + // selection toggle: one selection affordance, two lists. + row = row.child( + div().flex_shrink_0().child( + Button::new( + ElementId::from(SharedString::from(format!( + "review-comment-select-{}", + comment.id + ))), + if selected { + t!("fleet.review.comment_selected").to_string() + } else { + t!("fleet.review.comment_select").to_string() + }, + ) + .size(ButtonSize::Sm) + .variant(if selected { + ButtonVariant::Default + } else { + ButtonVariant::Outline + }) + .disabled(!can_anchor) + .on_click(move |_, _, cx| { + select_entity.update(cx, |this, cx| { + this.set_review_comment_selected(&select_id, !selected); + cx.notify(); + }); + }), + ), + ); + } + comments = comments.child(row); + } + // Unlike the staging sibling, this reason is shown only when a comment + // in the snapshot looks locally sendable. A review with nothing in + // `not_sent`/`refused` has no gap to explain, and a permanent + // "nothing to deliver" banner would be noise rather than an answer. + if let Err(reason) = delivery.control { + if review + .comments + .iter() + .any(|comment| review.comment_is_batch_actionable(&comment.id)) + { + comments = comments.child( + div().min_w(px(0.0)).overflow_hidden().child( + Alert::info() + .title(t!("fleet.review.delivery_withheld_title").to_string()) + .description(t!(review_delivery_withheld_key(reason)).to_string()), + ), + ); + } } let mut attention = div().flex().flex_col().gap(px(4.0)); @@ -1850,15 +1999,15 @@ impl FleetView { ); } if selected_notes > 0 { - // The selection is durable and ready, but Platform v2 advertises - // no capability for delivering it to the authorized session, so - // Fleet states that instead of offering a control that cannot - // honour its own at-most-once contract. + // A note is a local draft and is not on the server, so it cannot + // be delivered directly: batch delivery names server comment ids, + // which a note only acquires once it has been prepared and posted. + // The durable selection says which drafts the user means to send. notes = notes.child( div() .text_size(px(10.0)) - .text_color(ShellDeckColors::warning()) - .child(t!("fleet.review.batch_blocked", count = selected_notes).to_string()), + .text_color(ShellDeckColors::text_muted()) + .child(t!("fleet.review.notes_selected", count = selected_notes).to_string()), ); } @@ -1883,9 +2032,35 @@ impl FleetView { .to_string() }, ); - // External Git, agent-delivery, and pull-request actions remain absent: - // Platform v2 currently advertises only explicit check-rerun authority. + // External Git and pull-request actions remain absent: Platform v2 + // advertises check reruns and agent delivery, and ships the three + // pull-request slots empty because no provider adapter can preflight + // one yet. let mut effect_controls = div().flex().flex_wrap().gap(px(6.0)); + if delivery.control.is_ok() && !self.selected_review_comments.is_empty() { + let batch_entity = entity.clone(); + let selected_comments = self.selected_review_comments.len(); + effect_controls = effect_controls.child( + Button::new( + "prepare-review-batch-send", + t!("fleet.review.prepare_batch_send", count = selected_comments).to_string(), + ) + .size(ButtonSize::Sm) + .variant(ButtonVariant::Outline) + .disabled(!can_prepare) + .on_click(move |_, _, cx| { + batch_entity.update(cx, |this, cx| { + if let Err(refusal) = this.prepare_review_batch_send() { + this.review_receipt = Some(( + t!("fleet.review.state_refused").to_string(), + refusal.detail(), + )); + } + cx.notify(); + }); + }), + ); + } let capabilities = self .review_capabilities .as_ref() @@ -3796,10 +3971,11 @@ impl Render for FleetView { mod review_render_tests { use super::{ exact_review_target_index, fleet_uses_compact_layout, review_anchor_element_id, - review_dispatch_directive, review_lane_label_key, review_lane_variant, - review_preparation_gate, review_preview_withheld_key, review_rerun_control_gate, - review_staging_withheld_key, same_exact_review_snapshot, semantic_words, - visible_review_lanes, BadgeVariant, ReviewDispatchDirective, ReviewPrepareRefusal, + review_delivery_withheld_key, review_dispatch_directive, review_lane_label_key, + review_lane_variant, review_preparation_gate, review_preview_withheld_key, + review_rerun_control_gate, review_staging_withheld_key, same_exact_review_snapshot, + semantic_words, visible_review_lanes, BadgeVariant, ReviewDeliveryWithheld, + ReviewDispatchDirective, ReviewPrepareRefusal, }; use shelldeck_core::config::platform_review::PlatformReviewTarget; use shelldeck_core::config::workspace_catalog::{ @@ -4152,4 +4328,45 @@ mod review_render_tests { ); assert!(details.iter().all(|detail| !detail.trim().is_empty())); } + + // SDTEST-1866 — SDUC-495 + #[test] + fn every_delivery_withheld_reason_has_its_own_key_in_both_shipped_locales() { + // A surface that cannot tell "the server advertised nothing" from + // "this process has no custody lane" sends the user to the wrong fix, + // so the three reasons must never collapse onto one string. + let keys = ReviewDeliveryWithheld::ALL + .iter() + .map(|reason| review_delivery_withheld_key(*reason)) + .collect::>(); + assert_eq!(keys.len(), ReviewDeliveryWithheld::ALL.len()); + + for locale in ["fr", "en"] { + let copies = keys + .iter() + .map(|key| { + let copy = crate::t!(*key, locale = locale).to_string(); + assert_ne!(©, *key, "{locale} is missing {key}"); + copy + }) + .collect::>(); + assert_eq!(copies.len(), keys.len(), "{locale} reuses one explanation"); + // The delivery lane must not be explained with the staging lane's + // copy: they are withheld for different reasons and are fixed + // differently. + for key in [ + "fleet.review.staging_withheld_no_server_capability", + "fleet.review.staging_withheld_no_custody_lane", + ] { + assert!(!copies.contains(&crate::t!(key, locale = locale).to_string())); + } + let stale = + crate::t!("fleet.review.delivery_capability_stale", locale = locale).to_string(); + assert_ne!( + stale, + crate::t!("fleet.review.capability_stale", locale = locale).to_string(), + "{locale} tells a retracted advertisement and a changed rerun digest apart" + ); + } + } } diff --git a/docs/testing/USE_CASES.md b/docs/testing/USE_CASES.md index 1c9269dd..e93305e5 100644 --- a/docs/testing/USE_CASES.md +++ b/docs/testing/USE_CASES.md @@ -2906,15 +2906,41 @@ captured snapshot revision it was written against, plus a durable batch-delivery selection; a note captured on a superseded snapshot is retained but reported non-actionable rather than re-anchored, and it is stored under the same bounded, no-follow, cross-process-locked private boundary as review -custody. Batch delivery of a selection to the authorized session remains -unexposed and is stated as unavailable, because `ReviewCapabilities` advertises -only check reruns. A terminal CI check exposes rerun only when a separately +custody. A note is a local draft and carries no server comment id, so its +durable selection marks which drafts to prepare, never what is delivered. + +Batch delivery to the authorized session is exposed for exactly the comments +the server advertised in `agent_deliverable_comments` for this project, +workspace and snapshot revision. Each comment's id and expected revision are +sent verbatim from that capability, at the capability's own snapshot revision, +in the strictly ordered form the contract requires; the snapshot is consulted +only to prove the two reads agree and supplies no value that crosses the +network. A capability list for another coordinate, an entry the snapshot +contradicts or does not carry, a comment already delivered, a review that is +not fresh, and an empty advertised list each withhold the control with their +own stated reason. + +This lane carries no confirmation digest and no receipt correlation, and that +is deliberate. The target session is registry-owned and never on the wire, the +snapshot revision is fenced twice server-side, and the note set is fenced by +the server's own batch arm, so exactly-once falls out of the domain state +machine; a minted correlation would additionally make the client's own receipt +unfindable, because the host's receipt lookup skips a retained action carrying +one. What replaces the digest is the advertisement, so ShellDeck re-reads it +after every settlement rather than reusing it: a batch preview is revalidated +against the current advertisement before it crosses the dispatch fence, and the +persisted selection is retained against that advertisement rather than the +snapshot, so a settled delivery empties it instead of allowing a stale list to +be posted under a fresh idempotency key. + +A terminal CI check exposes rerun only when a separately fetched server capability matches the exact project, workspace, snapshot revision, check revision, authority, confirmation digest, and -receipt-correlation digest. Batch-to-agent, Git -proposal, and merge preview constructors remain inert core semantics: Fleet -does not expose those controls until their own explicit server capabilities and -custody lanes exist. Every exposed confirmation names the exact workspace and +receipt-correlation digest. Git proposal and merge preview constructors remain +inert core semantics: Fleet does not expose those controls until their own +explicit server capabilities and custody lanes exist, and the three +pull-request slots ship empty because no provider adapter can preflight one. +Every exposed confirmation names the exact workspace and captured snapshot or target revision before dispatch. The read side presents those files as one combined worktree: conflicted first, @@ -2926,9 +2952,9 @@ file, and hunk so a twice-listed file cannot route one lane's click to the other. The server's own stage, unstage, commit, and conflict-resolution proposals are shown as per-file observations with their admissibility, and the surface states which fence is missing instead of offering a control: Platform -v2 `ReviewCapabilities` advertises rerunnable checks only, defines no staging -capability, confirmation digest, or receipt-correlation digest, and the action -preview structurally refuses a confirmation on those four actions. Hunk-level +v2 `ReviewCapabilities` defines no staging capability, confirmation digest, or +receipt-correlation digest, and the action preview structurally refuses a +confirmation on those four actions. Hunk-level staging is not representable at all — a proposal names files, never hunks. Previews are decided from declared metadata, never from trust. Text is painted @@ -2943,8 +2969,9 @@ a declared size or raster above the client budget, and a kind disagreeing with its media type or content are each withheld with a distinct localized reason present in both shipped locales. -Before any exposed review mutation crosses the network — comment, approval or -confirmed rerun — ShellDeck persists the inert preview and then a dispatched +Before any exposed review mutation crosses the network — comment, approval, +confirmed rerun or unconfirmed batch delivery — ShellDeck persists the inert +preview and then a dispatched marker under a bounded, no-follow, cross-process-locked custody store. A workspace owns at most one non-terminal effect at a time, and preparation is refused outright when that store is unavailable. Accepted, unknown, or @@ -2963,6 +2990,35 @@ an unavailable adapter refuses before any effect. ## Change log +- **2026-08-29** — Expanded SDUC-495 with capability-fenced batch delivery of + review comments to the authorized session, and added SDTEST-1863..1866. The + item was blocked on a contract gap, not a client one: `ReviewCapabilities` + advertised only `rerunnable_checks`, so nothing could fence the control. + `bext-stack/automonique` PR #221 lands `agent_deliverable_comments`, and the + send control now exists only for the comments it names, with every coordinate + taken verbatim from it. + + This is the unconfirmed lane, deliberately. A rerun needs a digest because + the client names the target and the effect fires in a system the daemon does + not own; delivery reaches a registry-owned session that is never on the wire, + at a snapshot revision fenced twice server-side, over a note set fenced by + the server's batch arm. Minting a receipt correlation would have been worse + than redundant — the host's receipt lookup skips a retained action carrying + one, so the client would have made its own receipt unfindable. + + The advertisement is what replaces the digest, so it must be re-read after a + receipt settles rather than reused; a reused list is stale by construction, + because settling moves the comment out of `not_sent`/`refused` and bumps both + the snapshot revision and the comment's own. SDTEST-1863 demonstrates that + rather than asserting it: it replays the server's own eligibility rule, shows + the advertisement retracting exactly the delivered comment, and shows the + reused list refused even under a fresh idempotency key. + + The custody fence was not weakened to fit the lane. Batch delivery needs no + confirmation digest but is still an exposed mutation, and the custody disk + format knew only `AddComment`, `ApproveReview` and `RerunCheck` — so it could + not be recorded at all and would have refused at the fence. + - **2026-08-29** — Expanded SDUC-495 with durable line-anchored review notes and extended the custody fence to every exposed mutation; added SDTEST-1853..1855, SDTEST-1858 and SDTEST-1859. Comments and approvals diff --git a/docs/testing/tests-core.md b/docs/testing/tests-core.md index 1b44a817..f03c50ad 100644 --- a/docs/testing/tests-core.md +++ b/docs/testing/tests-core.md @@ -558,6 +558,9 @@ Existing: **0 tests**. | SDTEST-1854 | `config/platform_review/notes.rs::superseded_revision_and_foreign_anchor_are_never_actionable` | SDUC-495 | Green | A note captured on a superseded snapshot is retained but reported non-actionable rather than silently re-anchored; a line outside the exact hunk and an empty body are refused before persistence. | | SDTEST-1855 | `config/platform_review/notes.rs::oversized_note_document_is_rejected_without_replacement` | SDUC-495 | Green | An oversized note document fails closed on open and leaves the bytes on disk untouched. | | SDTEST-1858 | `config/platform_review/custody.rs::unconfirmed_comment_is_fenced_restart_safe_and_actor_attributed` | SDUC-495 | Green | An unconfirmed `AddComment` traverses the same durable custody lane as a confirmed rerun: a second comment cannot open a parallel effect, restart recovers lookup-only instead of reposting, and the terminal receipt retains the server actor. | +| SDTEST-1863 | `config/platform_review.rs::advertised_delivery_is_sent_verbatim_and_a_reused_advertisement_is_refused` | SDUC-495 | Green | Batch delivery sends exactly what `agent_deliverable_comments` advertised, at the capability's own snapshot revision and in the wire order the protocol requires, then demonstrates the retraction rather than asserting it: settling one delivery is replayed through the server's own eligibility rule, the re-read advertisement drops only that comment, and the preview built from the superseded list is refused locally. A fresh preview over the same batch and the same advertised revisions carries a new idempotency key and is refused identically, which is what makes a confirmation digest unnecessary here. A capability minted at another snapshot revision, and a sendable comment that was simply never advertised, both refuse at construction. | +| SDTEST-1864 | `config/platform_review/delivery.rs::every_delivery_fence_fails_closed_with_its_own_distinct_reason` | SDUC-495 | Green | Each delivery fence fails closed with its own reason: an absent capability response, one attributed to another project or a superseded snapshot revision, an exact response advertising nothing, and a missing custody lane are four distinct answers rather than one. Torn reads between the capability list and the snapshot — a disagreeing comment revision, an unknown comment, a foreign review authority, an already-sent comment, a stale review — drop the entry. `advertised_agent_deliveries` is asserted directly and not only through the projection, because the action preview calls it on its own. | +| SDTEST-1865 | `config/platform_review/custody.rs::unconfirmed_batch_delivery_is_fenced_and_survives_the_disk_round_trip` | SDUC-495 | Green | The unconfirmed batch delivery takes the same durable at-most-once lane as every other exposed mutation despite carrying no confirmation digest: a second delivery cannot open a parallel effect, a restart before dispatch reports it never started and after dispatch is lookup-only, the exact comment set and revisions survive the disk round trip, and the terminal receipt retains the server actor. Before this slice the custody disk format knew only `AddComment`, `ApproveReview` and `RerunCheck`, so a batch delivery could not be recorded at all. | --- diff --git a/docs/testing/tests-ui-and-app.md b/docs/testing/tests-ui-and-app.md index 1ad4b533..ddd16f34 100644 --- a/docs/testing/tests-ui-and-app.md +++ b/docs/testing/tests-ui-and-app.md @@ -544,6 +544,7 @@ parallel `cargo test`. | SDTEST-1851 | `fleet_view.rs::review_render_tests::sdtest_1851_every_withheld_reason_has_distinct_copy_in_both_locales` | SDUC-495 | Green | Every declared lane, preview-withheld and staging-withheld state resolves to a distinct non-empty localized label present in both `fr.toml` and `en.toml`. A new protocol variant therefore cannot reach the user as a raw key or silently reuse another state's copy. | | SDTEST-1852 | `fleet_view.rs::review_render_tests::sdtest_1852_anchor_identity_survives_a_file_listed_in_two_lanes` | SDUC-495 | Green | A partially staged file renders in two lanes, so the anchor button identity binds lane, file and hunk. GPUI routes a click by element id: colliding ids would select the other lane's anchor. The identity is also stable across renders. | | SDTEST-1859 | `fleet_view.rs::review_render_tests::preparation_requires_custody_for_every_action_family` | SDUC-495 | Green | Preparation of any review mutation now requires the durable custody store, not merely an idle non-ambiguous surface; the rerun gate stays strictly narrower, and a custody refusal is reported with copy distinct from a moved snapshot so the user's next step differs. | +| SDTEST-1866 | `fleet_view.rs::review_render_tests::every_delivery_withheld_reason_has_its_own_key_in_both_shipped_locales` | SDUC-495 | Green | Each batch-delivery withheld reason resolves to its own non-empty localized explanation in both `fr.toml` and `en.toml`, never reuses another delivery reason's copy, and never borrows the staging lane's copy — the two are withheld for different reasons and fixed differently. A retracted delivery advertisement is also reported with copy distinct from a changed rerun digest. | | SDTEST-1811 | `workspace/workspaces_tests.rs::sdtest_1811_retained_gpui_attention_opens_exact_workspace_pane_and_tab` | SDUC-493 | Green | A retained GPUI workspace renders the typed state, unread state, and nested-agent path. After A→B, activating A's captured attention revision returns to its exact retained pane/tab and marks only that revision read; a newer replay makes the old activation stale and stays unread. | | SDTEST-1830 | `workspace/fleet.rs::orchestration_attention_remains_discoverable_without_a_review_snapshot` | SDUC-493 | Green | The exact typed review-not-found refusal is treated as authoritative review absence, so orchestration/provider attention inventory loading continues; transport/refusal ambiguity remains fail-closed and retains prior board custody. | | SDTEST-1831 | `main.rs::same_process_attention_notifications_activate_only_explicit_open_actions` | SDUC-493 | Green | The pure callback admission policy accepts only the native default/open action strings; dismissal and unknown actions cannot enqueue navigation. Production wiring retains the handle in its blocking same-process worker, while native delivery/activation remains a release acceptance check rather than something this classifier unit test simulates. Cold-launch deep links remain deliberately out of scope. |