diff --git a/Cargo.lock b/Cargo.lock index c9d98e51..058a116e 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=7d2fad53558ccb38ef3fdb12945974f75a0e73ce#7d2fad53558ccb38ef3fdb12945974f75a0e73ce" +source = "git+https://github.com/bext-stack/automonique?rev=524073c3a55ec73d57e4b05abf5ba3eb0abd4cb7#524073c3a55ec73d57e4b05abf5ba3eb0abd4cb7" 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=7d2fad53558ccb38ef3fdb12945974f75a0e73ce#7d2fad53558ccb38ef3fdb12945974f75a0e73ce" +source = "git+https://github.com/bext-stack/automonique?rev=524073c3a55ec73d57e4b05abf5ba3eb0abd4cb7#524073c3a55ec73d57e4b05abf5ba3eb0abd4cb7" [[package]] name = "av-scenechange" diff --git a/Cargo.toml b/Cargo.toml index a47a6638..cfa5787e 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 = "7d2fad53558ccb38ef3fdb12945974f75a0e73ce" } -automonique-protocol = { git = "https://github.com/bext-stack/automonique", rev = "7d2fad53558ccb38ef3fdb12945974f75a0e73ce" } +automonique-platform-client = { git = "https://github.com/bext-stack/automonique", rev = "524073c3a55ec73d57e4b05abf5ba3eb0abd4cb7" } +automonique-protocol = { git = "https://github.com/bext-stack/automonique", rev = "524073c3a55ec73d57e4b05abf5ba3eb0abd4cb7" } 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 9180932e..b1729168 100644 --- a/crates/shelldeck-core/locales/en.toml +++ b/crates/shelldeck-core/locales/en.toml @@ -1249,9 +1249,16 @@ fleet.review.lane_untracked = "Untracked" 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 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_title = "Staging is unavailable" +fleet.review.staging_withheld_no_server_capability = "No capability response is attributed to this project, workspace and snapshot revision: nothing has been advertised, so no staging control is offered." +fleet.review.staging_withheld_no_stageable_proposal = "The server advertises no staging and no conflict resolution for this snapshot: the registry binding does not grant it, the repository could not be read, or the worktree is in a state no write can fence exactly once. The advertisement is re-read on every refresh, including after a write." fleet.review.staging_withheld_no_custody_lane = "Durable custody is unavailable in this process: no staging control is offered." +fleet.review.staging_observation = "Server fence, HEAD %{head} · index %{index}" +fleet.review.prepare_stage = "Stage" +fleet.review.prepare_unstage = "Unstage" +fleet.review.prepare_commit = "Commit" +fleet.review.resolve_keep_current = "Keep current" +fleet.review.resolve_keep_incoming = "Keep incoming" fleet.review.preview_text = "Sanitized text preview" fleet.review.preview_image = "Image %{media} · %{width}×%{height} px · %{bytes} bytes" fleet.review.preview_image_box = "Bounded preview, never decoded" diff --git a/crates/shelldeck-core/locales/fr.toml b/crates/shelldeck-core/locales/fr.toml index 4674ea19..142b0f26 100644 --- a/crates/shelldeck-core/locales/fr.toml +++ b/crates/shelldeck-core/locales/fr.toml @@ -1249,9 +1249,16 @@ fleet.review.lane_untracked = "Non suivi" 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 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_title = "Indexation indisponible" +fleet.review.staging_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’indexation n’est proposée." +fleet.review.staging_withheld_no_stageable_proposal = "Le serveur n’annonce aucune indexation ni résolution de conflit pour cet instantané : la liaison du registre ne l’accorde pas, le dépôt n’a pas pu être lu, ou l’arbre de travail est dans un état qu’aucune écriture ne peut clôturer exactement une fois. L’annonce est relue à chaque rafraîchissement, y compris après une écriture." 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.staging_observation = "Barrière serveur, HEAD %{head} · index %{index}" +fleet.review.prepare_stage = "Indexer" +fleet.review.prepare_unstage = "Désindexer" +fleet.review.prepare_commit = "Valider" +fleet.review.resolve_keep_current = "Garder la version actuelle" +fleet.review.resolve_keep_incoming = "Garder la version entrante" fleet.review.preview_text = "Aperçu texte assaini" fleet.review.preview_image = "Image %{media} · %{width}×%{height} px · %{bytes} octets" fleet.review.preview_image_box = "Aperçu borné, jamais décodé" diff --git a/crates/shelldeck-core/src/config/platform.rs b/crates/shelldeck-core/src/config/platform.rs index daf59d78..7ee09118 100644 --- a/crates/shelldeck-core/src/config/platform.rs +++ b/crates/shelldeck-core/src/config/platform.rs @@ -1684,7 +1684,8 @@ mod tests { use automonique_protocol::platform_v2_transport::{ PlatformNegotiationResponse, PlatformV2Refusal, PlatformV2Request, PlatformV2Response, ReviewCapabilities, ReviewCheckRerunCapability, ReviewConfirmationDigest, - ReviewPullRequestCapabilities, ReviewReceiptCorrelationDigest, + ReviewGitStagingCapabilities, ReviewPullRequestCapabilities, + ReviewReceiptCorrelationDigest, }; let snapshot = decode_review_snapshot(include_bytes!( @@ -1718,6 +1719,7 @@ mod tests { .unwrap()], Vec::new(), ReviewPullRequestCapabilities::default(), + ReviewGitStagingCapabilities::default(), ) .unwrap(); let negotiated = || { diff --git a/crates/shelldeck-core/src/config/platform_review.rs b/crates/shelldeck-core/src/config/platform_review.rs index 2183e68f..f8bef462 100644 --- a/crates/shelldeck-core/src/config/platform_review.rs +++ b/crates/shelldeck-core/src/config/platform_review.rs @@ -19,7 +19,9 @@ pub use automonique_protocol::platform_v2_review::{ }; pub use automonique_protocol::platform_v2_transport::{ ReviewAgentDeliveryCapability, ReviewCapabilities, ReviewCheckRerunCapability, - ReviewConfirmationDigest, ReviewPullRequestCapabilities, ReviewReceiptCorrelationDigest, + ReviewConfirmationDigest, ReviewConflictResolutionCapability, ReviewGitStagingCapabilities, + ReviewIndexDigest, ReviewPullRequestCapabilities, ReviewPullRequestMergeCapability, + ReviewReceiptCorrelationDigest, ReviewStagingCapability, }; use automonique_protocol::primitives::Revision; use std::collections::BTreeSet; @@ -43,10 +45,12 @@ pub use notes::{PlatformReviewNote, PlatformReviewNoteStore, ReviewNoteError}; mod worktree; pub use worktree::{ - advertised_staging_capability, review_safe_preview, review_safe_text, review_staging_control, - ReviewPreviewWithheld, ReviewSafeHtml, ReviewSafeImage, ReviewSafePreview, ReviewSafeText, - ReviewStagingProposal, ReviewStagingWithheld, ReviewWorktreeFile, ReviewWorktreeHunk, - ReviewWorktreeLane, ReviewWorktreeLaneGroup, ReviewWorktreeProjection, MAX_SAFE_PREVIEW_BYTES, + advertised_conflict_resolutions, advertised_staging_proposals, review_safe_preview, + review_safe_text, review_staging_control, AdvertisedReviewConflictResolution, + AdvertisedReviewStaging, ReviewPreviewWithheld, ReviewSafeHtml, ReviewSafeImage, + ReviewSafePreview, ReviewSafeText, ReviewStagingProposal, ReviewStagingWithheld, + ReviewWorktreeFile, ReviewWorktreeHunk, ReviewWorktreeLane, ReviewWorktreeLaneGroup, + ReviewWorktreeObservation, ReviewWorktreeProjection, MAX_SAFE_PREVIEW_BYTES, MAX_SAFE_PREVIEW_EDGE, MAX_SAFE_PREVIEW_LINES, MAX_SAFE_PREVIEW_LINE_CHARS, MAX_SAFE_PREVIEW_PIXELS, SAFE_PREVIEW_BOX_EDGE, }; @@ -233,30 +237,125 @@ impl PlatformReviewActionPreview { ) } - pub fn apply_proposal( + /// Prepare one confirmed index-level staging transition. + /// + /// The *kind* is taken from the capability, never from the snapshot's + /// proposal. Both agree on a coherent read — [`advertised_staging_proposals`] + /// checks that — but the value the confirmation digest was minted over is + /// this one, so this is the one that crosses the wire. + /// + /// This is the confirmed lane. `Stage`, `Unstage` and `Commit` now require + /// a server-minted confirmation, and their unconfirmed spelling cannot be + /// encoded at all, so an unadvertised proposal refuses here rather than + /// being posted and refused remotely. A deployment withholding the commit + /// grant advertises no `Commit` entry, so this refuses for it while + /// admitting its `Stage` and `Unstage` siblings — the withholding is the + /// absence, and nothing here can invent the missing entry. + pub fn stage_proposal( target: PlatformReviewTarget, review: &PlatformReviewSemantic, + capabilities: &ReviewCapabilities, proposal_id: &str, ) -> Result { validate_review_target(&target, review)?; - let proposal = review - .proposals + let advertised = advertised_staging_proposals(Some(capabilities), &target, review); + if !advertised .iter() - .find(|proposal| proposal.id == proposal_id) - .filter(|proposal| review.proposal_is_actionable(&proposal.id)) - .ok_or("review proposal is not actionable")?; - let proposal_id = ReviewProposalId::new(proposal.id.clone()) + .any(|entry| entry.proposal_id == proposal_id) + { + return Err("review staging proposal is not advertised"); + } + let capability = capabilities + .staging() + .iter() + .find(|capability| capability.proposal_id().as_str() == proposal_id) + .ok_or("review staging capability is unavailable or stale")?; + let id = ReviewProposalId::new(proposal_id.to_owned()) .map_err(|_| "review proposal identity is invalid")?; - let action = match proposal.kind { - ReviewProposalKind::Stage => ReviewAction::Stage { proposal_id }, - ReviewProposalKind::Unstage => ReviewAction::Unstage { proposal_id }, - ReviewProposalKind::Commit => ReviewAction::Commit { proposal_id }, + let action = match capability.kind() { + ReviewProposalKind::Stage => ReviewAction::Stage { proposal_id: id }, + ReviewProposalKind::Unstage => ReviewAction::Unstage { proposal_id: id }, + ReviewProposalKind::Commit => ReviewAction::Commit { proposal_id: id }, + // Unreachable: `ReviewStagingCapability::new` refuses this kind, + // because collapsing a conflict writes worktree bytes rather than + // index entries and carries its own capability. ReviewProposalKind::ResolveConflict => { return Err("conflict resolution requires an explicit resolution") } }; + let confirmation = ReviewActionConfirmation::new( + capability.confirmation_digest().clone(), + capabilities.workspace_revision(), + capability.receipt_correlation_digest().clone(), + ); let nonce = review_nonce(); - Self::review_action(target, review.revision, action, &nonce, None) + Self::review_action( + target, + capabilities.snapshot_revision(), + action, + &nonce, + Some(confirmation), + ) + } + + /// Prepare one confirmed collapse of a conflicted file to a recorded side. + /// + /// `resolution` is not a free choice: the server advertises one entry per + /// side git actually recorded, and this refuses any other. That is why the + /// side has to arrive as an argument rather than be defaulted — the + /// confirmation digest commits to it, so a control that guessed would + /// carry a digest for the other blob. + /// + /// No content crosses this wire in either direction. What lands is exactly + /// the blob git is already holding as stage 2 (`keep_current`) or stage 3 + /// (`keep_incoming`) for the one path the action names. + pub fn resolve_conflict( + target: PlatformReviewTarget, + review: &PlatformReviewSemantic, + capabilities: &ReviewCapabilities, + proposal_id: &str, + file_id: &str, + resolution: ConflictResolution, + ) -> Result { + validate_review_target(&target, review)?; + let advertised = advertised_conflict_resolutions(Some(capabilities), &target, review); + if !advertised.iter().any(|entry| { + entry.proposal_id == proposal_id + && entry.file_id == file_id + && entry.resolution == resolution + }) { + return Err("review conflict resolution is not advertised"); + } + let capability = capabilities + .conflict_resolutions() + .iter() + .find(|capability| { + capability.proposal_id().as_str() == proposal_id + && capability.file_id().as_str() == file_id + && capability.resolution() == resolution + }) + .ok_or("review conflict resolution capability is unavailable or stale")?; + let confirmation = ReviewActionConfirmation::new( + capability.confirmation_digest().clone(), + capabilities.workspace_revision(), + capability.receipt_correlation_digest().clone(), + ); + let nonce = review_nonce(); + Self::review_action( + target, + capabilities.snapshot_revision(), + ReviewAction::ResolveConflict { + proposal_id: ReviewProposalId::new(proposal_id.to_owned()) + .map_err(|_| "review proposal identity is invalid")?, + file_id: automonique_protocol::platform_v2_review::ReviewFileId::new( + file_id.to_owned(), + ) + .map_err(|_| "review file identity is invalid")?, + resolution, + }, + &nonce, + Some(confirmation), + ) } pub fn rerun_check( @@ -307,36 +406,63 @@ impl PlatformReviewActionPreview { ) } + /// Prepare the confirmed merge of the advertised pull request. + /// + /// This became a confirmed lane with the same protocol revision that + /// earned the staging capability: `MergePullRequest` is now inside + /// `requires_confirmation`, so its unconfirmed spelling no longer encodes + /// and the previous constructor minted a preview the transport could not + /// have sent. Every coordinate is therefore taken from the capability the + /// server minted, and the snapshot is consulted only to prove the two + /// reads agree. + /// + /// The server ships this slot empty for every workspace today — no + /// provider adapter can preflight a pull-request write — so this refuses + /// in production, which is the honest answer rather than a silent one. pub fn merge_pull_request( target: PlatformReviewTarget, review: &PlatformReviewSemantic, + capabilities: &ReviewCapabilities, ) -> Result { validate_review_target(&target, review)?; - let pull = &review.pull_request; if !review.pull_request_is_mergeable() { return Err("pull request is not mergeable"); } - let id = pull - .id - .as_ref() - .ok_or("pull request identity is unavailable")?; - let head = pull - .head_revision - .as_ref() - .ok_or("pull request head revision is unavailable")?; + if capabilities.project() != &target.project + || capabilities.workspace() != &target.workspace + || capabilities.snapshot_revision() != review.revision + { + return Err("review capabilities are stale or belong to another workspace"); + } + let pull = &review.pull_request; + let capability = capabilities + .merge_pull_request() + .filter(|capability| { + Some(capability.pull_request_id().as_str()) == pull.id.as_deref() + && capability.expected_pull_request_revision() + == pull.freshness.observed_revision + && Some(capability.expected_head_revision().as_str()) + == pull.head_revision.as_deref() + && capability.authority().kind() == pull.authority.kind + && capability.authority().id().as_str() == pull.authority.id + }) + .ok_or("pull request merge capability is unavailable or stale")?; + let confirmation = ReviewActionConfirmation::new( + capability.confirmation_digest().clone(), + capabilities.workspace_revision(), + capability.receipt_correlation_digest().clone(), + ); let nonce = review_nonce(); Self::review_action( target, - review.revision, + capabilities.snapshot_revision(), ReviewAction::MergePullRequest { - pull_request_id: PullRequestId::new(id.clone()) - .map_err(|_| "pull request identity is invalid")?, - expected_pull_request_revision: pull.freshness.observed_revision, - expected_head_revision: ReviewField::new(head.clone()) - .map_err(|_| "pull request head revision is invalid")?, + pull_request_id: capability.pull_request_id().clone(), + expected_pull_request_revision: capability.expected_pull_request_revision(), + expected_head_revision: capability.expected_head_revision().clone(), }, &nonce, - None, + Some(confirmation), ) } @@ -350,7 +476,12 @@ impl PlatformReviewActionPreview { action .validate_client_shape() .map_err(|_| "review action is invalid")?; - if matches!(action, ReviewAction::RerunCheck { .. }) != confirmation.is_some() { + // Ask the contract rather than restate it. The confirmed set grew from + // the check rerun alone to the three pull-request writes and the four + // staging writes, and a local `matches!` would have kept accepting the + // unconfirmed spelling of each new member long after the transport + // stopped encoding it. + if action.requires_confirmation() != confirmation.is_some() { return Err("review action confirmation does not match its action"); } let idempotency_key = IdempotencyKey::new(format!("shelldeck-review-{nonce}")) @@ -451,6 +582,90 @@ impl PlatformReviewActionPreview { }) }) } + // The staging lane is fenced by its digest, and the digest is what + // makes a moved worktree refuse *here* rather than at the daemon. + // The server mints it over the commit `HEAD` resolved to, the + // whole index, and every named path's objects and stat identity, + // so any of those moving between two capability reads yields a + // different digest and no match. Comparing the exposed + // `expected_head_revision` and `expected_index_digest` separately + // would restate a subset of the same commitment. + // + // Only a fresh capability document can supply that comparison, + // which is why every staging preview reports + // `requires_capability_revalidation`. + ( + ReviewAction::Stage { proposal_id } + | ReviewAction::Unstage { proposal_id } + | ReviewAction::Commit { proposal_id }, + Some(confirmation), + ) => { + let kind = match &self.action { + ReviewAction::Stage { .. } => ReviewProposalKind::Stage, + ReviewAction::Unstage { .. } => ReviewProposalKind::Unstage, + _ => ReviewProposalKind::Commit, + }; + self.capability_coordinates_match(capabilities) + && capabilities.workspace_revision() + == confirmation.expected_workspace_revision() + && capabilities.staging().iter().any(|capability| { + capability.proposal_id() == proposal_id + && capability.kind() == kind + && capability.confirmation_digest() + == confirmation.confirmation_digest() + && capability.receipt_correlation_digest() + == confirmation.receipt_correlation_digest() + }) + } + // The side is part of the identity, not a parameter of it: two + // entries for one file differ only by `resolution`, and each + // carries the digest minted over the blob that side would write. + ( + ReviewAction::ResolveConflict { + proposal_id, + file_id, + resolution, + }, + Some(confirmation), + ) => { + self.capability_coordinates_match(capabilities) + && capabilities.workspace_revision() + == confirmation.expected_workspace_revision() + && capabilities + .conflict_resolutions() + .iter() + .any(|capability| { + capability.proposal_id() == proposal_id + && capability.file_id() == file_id + && capability.resolution() == *resolution + && capability.confirmation_digest() + == confirmation.confirmation_digest() + && capability.receipt_correlation_digest() + == confirmation.receipt_correlation_digest() + }) + } + ( + ReviewAction::MergePullRequest { + pull_request_id, + expected_pull_request_revision, + expected_head_revision, + }, + Some(confirmation), + ) => { + self.capability_coordinates_match(capabilities) + && capabilities.workspace_revision() + == confirmation.expected_workspace_revision() + && capabilities.merge_pull_request().is_some_and(|capability| { + capability.pull_request_id() == pull_request_id + && capability.expected_pull_request_revision() + == *expected_pull_request_revision + && capability.expected_head_revision() == expected_head_revision + && capability.confirmation_digest() + == confirmation.confirmation_digest() + && capability.receipt_correlation_digest() + == confirmation.receipt_correlation_digest() + }) + } (_, None) => true, _ => false, } @@ -718,6 +933,37 @@ impl PlatformReviewSemantic { }) } + /// Whether one conflicted file inside one proposal may be collapsed to a + /// side git recorded. + /// + /// Deliberately not folded into [`Self::proposal_is_actionable`], which + /// answers a different question: staging moves index entries for a whole + /// proposal and is refused outright while any of its files is unmerged, + /// whereas this is admissible *only* while the one named file is unmerged. + /// The two predicates are near-inverses, so sharing one would make each + /// caller re-derive which half it meant. + /// + /// Which side may be written is never decided here: the server advertises + /// one entry per side it actually recorded, and the client may name no + /// other. + #[must_use] + pub fn conflict_resolution_is_actionable(&self, proposal_id: &str, file_id: &str) -> bool { + self.proposals + .iter() + .find(|proposal| proposal.id == proposal_id) + .is_some_and(|proposal| { + proposal.kind == ReviewProposalKind::ResolveConflict + && proposal + .authority + .as_ref() + .is_some_and(|authority| authority.kind == ReviewAuthorityKind::Git) + && proposal.files.iter().any(|id| id == file_id) + && self.files.iter().any(|file| { + file.id == file_id && file.conflict == ConflictState::Unresolved + }) + }) + } + #[must_use] pub fn check_is_rerunnable(&self, check_id: &str) -> bool { self.checks.iter().any(|check| { @@ -1194,6 +1440,7 @@ mod tests { Vec::new(), advertised, ReviewPullRequestCapabilities::default(), + ReviewGitStagingCapabilities::default(), ) .unwrap() } @@ -1286,6 +1533,86 @@ mod tests { .unwrap()], Vec::new(), ReviewPullRequestCapabilities::default(), + ReviewGitStagingCapabilities::default(), + ) + .unwrap() + } + + /// The advertisement a coherent server would mint for this snapshot's + /// first proposal, at the `HEAD` and index its preflight read. + fn staging_capabilities( + target: &PlatformReviewTarget, + review: &PlatformReviewSemantic, + ) -> ReviewCapabilities { + let proposal = &review.proposals[0]; + ReviewCapabilities::new( + target.project.clone(), + target.workspace.clone(), + review.revision, + Revision::new(91).unwrap(), + Vec::new(), + Vec::new(), + ReviewPullRequestCapabilities::default(), + ReviewGitStagingCapabilities { + staging: vec![ReviewStagingCapability::new( + ReviewProposalId::new(proposal.id.clone()).unwrap(), + proposal.kind, + ReviewField::new("4".repeat(40)).unwrap(), + ReviewIndexDigest::new("a".repeat(64)).unwrap(), + ReviewAuthority::new( + ReviewAuthorityKind::Git, + automonique_protocol::platform_v2_review::ReviewAuthorityId::new( + proposal.authority.as_ref().unwrap().id.clone(), + ) + .unwrap(), + ), + ReviewConfirmationDigest::new("c".repeat(64)).unwrap(), + ReviewReceiptCorrelationDigest::new("d".repeat(64)).unwrap(), + ) + .unwrap()], + conflict_resolutions: Vec::new(), + }, + ) + .unwrap() + } + + /// The merge slot the server ships empty in production. It exists here + /// because `MergePullRequest` is a confirmed lane now, so the unconfirmed + /// spelling cannot be constructed at all. + fn merge_capabilities( + target: &PlatformReviewTarget, + review: &PlatformReviewSemantic, + ) -> ReviewCapabilities { + let pull = &review.pull_request; + ReviewCapabilities::new( + target.project.clone(), + target.workspace.clone(), + review.revision, + Revision::new(91).unwrap(), + Vec::new(), + Vec::new(), + ReviewPullRequestCapabilities { + merge: Some( + ReviewPullRequestMergeCapability::new( + PullRequestId::new(pull.id.clone().unwrap()).unwrap(), + pull.freshness.observed_revision, + ReviewField::new(pull.head_revision.clone().unwrap()).unwrap(), + pull.readiness, + ReviewAuthority::new( + pull.authority.kind, + automonique_protocol::platform_v2_review::ReviewAuthorityId::new( + pull.authority.id.clone(), + ) + .unwrap(), + ), + ReviewConfirmationDigest::new("e".repeat(64)).unwrap(), + ReviewReceiptCorrelationDigest::new("f".repeat(64)).unwrap(), + ) + .unwrap(), + ), + ..ReviewPullRequestCapabilities::default() + }, + ReviewGitStagingCapabilities::default(), ) .unwrap() } @@ -1681,9 +2008,10 @@ mod tests { }) ); - let proposal = PlatformReviewActionPreview::apply_proposal( + let proposal = PlatformReviewActionPreview::stage_proposal( target.clone(), &semantic, + &staging_capabilities(&target, &semantic), &semantic.proposals[0].id, ) .unwrap(); @@ -1704,6 +2032,16 @@ mod tests { proposal_id: semantic.proposals[0].id.clone(), }) ); + // Staging is a confirmed lane: the digests are the server's, verbatim. + let staging_confirmation = proposal.confirmation().unwrap(); + assert_eq!( + staging_confirmation.confirmation_digest().as_str(), + "c".repeat(64) + ); + assert_eq!( + staging_confirmation.receipt_correlation_digest().as_str(), + "d".repeat(64) + ); let rerun = PlatformReviewActionPreview::rerun_check( target.clone(), @@ -1735,8 +2073,12 @@ mod tests { }) ); - let merge = - PlatformReviewActionPreview::merge_pull_request(target.clone(), &semantic).unwrap(); + let merge = PlatformReviewActionPreview::merge_pull_request( + target.clone(), + &semantic, + &merge_capabilities(&target, &semantic), + ) + .unwrap(); assert_eq!(merge.target(), &target); assert_eq!(merge.expected_revision(), semantic.revision); assert_eq!( @@ -1772,9 +2114,24 @@ mod tests { }) ); + let merge_confirmation = merge.confirmation().unwrap(); + assert_eq!( + merge_confirmation.confirmation_digest().as_str(), + "e".repeat(64) + ); + assert_eq!( + merge_confirmation.receipt_correlation_digest().as_str(), + "f".repeat(64) + ); + let mut stale = semantic.clone(); stale.pull_request.freshness.state = ReviewFreshnessState::Stale; - assert!(PlatformReviewActionPreview::merge_pull_request(target.clone(), &stale).is_err()); + assert!(PlatformReviewActionPreview::merge_pull_request( + target.clone(), + &stale, + &merge_capabilities(&target, &semantic), + ) + .is_err()); stale = semantic.clone(); stale.checks[0].freshness.state = ReviewFreshnessState::Stale; assert!(PlatformReviewActionPreview::rerun_check( @@ -1792,9 +2149,10 @@ mod tests { .find(|file| file.id == proposal_file) .unwrap() .conflict = ConflictState::Unresolved; - assert!(PlatformReviewActionPreview::apply_proposal( + assert!(PlatformReviewActionPreview::stage_proposal( target.clone(), &conflicted, + &staging_capabilities(&target, &semantic), &conflicted.proposals[0].id, ) .is_err()); @@ -1818,9 +2176,10 @@ mod tests { &[wrong_authority.comments[0].id.clone()], ) .is_err()); - assert!(PlatformReviewActionPreview::apply_proposal( + assert!(PlatformReviewActionPreview::stage_proposal( target.clone(), &wrong_authority, + &staging_capabilities(&target, &semantic), &wrong_authority.proposals[0].id, ) .is_err()); @@ -1831,10 +2190,12 @@ mod tests { &wrong_authority.checks[0].id, ) .is_err()); - assert!( - PlatformReviewActionPreview::merge_pull_request(target.clone(), &wrong_authority,) - .is_err() - ); + assert!(PlatformReviewActionPreview::merge_pull_request( + target.clone(), + &wrong_authority, + &merge_capabilities(&target, &semantic), + ) + .is_err()); let foreign_target = PlatformReviewTarget { project: target.project.clone(), workspace: WorkContextIdentity::parse_local( @@ -1843,12 +2204,16 @@ mod tests { ) .unwrap(), }; - assert!( - PlatformReviewActionPreview::merge_pull_request(foreign_target, &semantic).is_err() - ); - assert!(PlatformReviewActionPreview::apply_proposal( + assert!(PlatformReviewActionPreview::merge_pull_request( + foreign_target, + &semantic, + &merge_capabilities(&target, &semantic), + ) + .is_err()); + assert!(PlatformReviewActionPreview::stage_proposal( target.clone(), &semantic, + &staging_capabilities(&target, &semantic), "proposal-missing", ) .is_err()); diff --git a/crates/shelldeck-core/src/config/platform_review/custody.rs b/crates/shelldeck-core/src/config/platform_review/custody.rs index 8f94428e..7199cb90 100644 --- a/crates/shelldeck-core/src/config/platform_review/custody.rs +++ b/crates/shelldeck-core/src/config/platform_review/custody.rs @@ -314,7 +314,11 @@ impl ReviewCustodyRecord { .as_ref() .map(ReviewConfirmationDisk::to_confirmation) .transpose()?; - if matches!(action, ReviewAction::RerunCheck { .. }) != confirmation.is_some() { + // Same contract predicate the preview constructor uses, for the same + // reason: a record whose confirmation no longer matches its family is + // a document that cannot be replayed onto the wire, so it is rejected + // as invalid rather than recovered into an unsendable preview. + if action.requires_confirmation() != confirmation.is_some() { return Err(ReviewCustodyError::DocumentInvalid); } action @@ -359,6 +363,48 @@ enum ReviewActionDisk { BatchSendCommentsToAgent { comments: Vec, }, + /// The three confirmed index-level staging lanes. + /// + /// Each is a separate variant rather than one variant carrying a `kind` + /// string, because the three are separately granted: an operator withholds + /// committing independently of index writes, and a recovered plan that + /// could be re-read as another kind would let the narrower grant replay as + /// the wider one. + /// + /// The disk format did not understand these before this slice, exactly as + /// it did not understand the batch delivery before PR #161: preparation + /// would have returned "unsupported action" and the dispatch would have + /// died at the custody fence. + Stage { + proposal_id: String, + }, + Unstage { + proposal_id: String, + }, + Commit { + proposal_id: String, + }, + /// Collapsing one conflicted path to one side git already recorded. + /// + /// The side is persisted because it is part of the effect's identity: it + /// decides which blob lands, and the confirmation digest was minted over + /// it. A record that lost it could be replayed as the other side. + ResolveConflict { + proposal_id: String, + file_id: String, + resolution: String, + }, + /// The confirmed pull-request merge. + /// + /// Unreachable in production — the server advertises no merge capability — + /// but the record has to exist for the same reason as the others: the + /// action is now confirmed, so it crosses the durable fence, and a family + /// the format cannot spell fails at `prepare` rather than at the wire. + MergePullRequest { + pull_request_id: String, + expected_pull_request_revision: u64, + expected_head_revision: String, + }, } /// One comment target inside a persisted batch delivery. @@ -405,6 +451,33 @@ impl ReviewActionDisk { }) .collect(), }, + ReviewAction::Stage { proposal_id } => Self::Stage { + proposal_id: proposal_id.as_str().to_owned(), + }, + ReviewAction::Unstage { proposal_id } => Self::Unstage { + proposal_id: proposal_id.as_str().to_owned(), + }, + ReviewAction::Commit { proposal_id } => Self::Commit { + proposal_id: proposal_id.as_str().to_owned(), + }, + ReviewAction::ResolveConflict { + proposal_id, + file_id, + resolution, + } => Self::ResolveConflict { + proposal_id: proposal_id.as_str().to_owned(), + file_id: file_id.as_str().to_owned(), + resolution: resolution.as_str().to_owned(), + }, + ReviewAction::MergePullRequest { + pull_request_id, + expected_pull_request_revision, + expected_head_revision, + } => Self::MergePullRequest { + pull_request_id: pull_request_id.as_str().to_owned(), + expected_pull_request_revision: expected_pull_request_revision.get(), + expected_head_revision: expected_head_revision.as_str().to_owned(), + }, _ => return Err(ReviewCustodyError::UnsupportedAction), }) } @@ -469,10 +542,51 @@ impl ReviewActionDisk { .collect::, ReviewCustodyError>>()?, } } + Self::Stage { proposal_id } => ReviewAction::Stage { + proposal_id: disk_proposal_id(proposal_id)?, + }, + Self::Unstage { proposal_id } => ReviewAction::Unstage { + proposal_id: disk_proposal_id(proposal_id)?, + }, + Self::Commit { proposal_id } => ReviewAction::Commit { + proposal_id: disk_proposal_id(proposal_id)?, + }, + Self::ResolveConflict { + proposal_id, + file_id, + resolution, + } => ReviewAction::ResolveConflict { + proposal_id: disk_proposal_id(proposal_id)?, + file_id: ReviewFileId::new(file_id.clone()) + .map_err(|_| ReviewCustodyError::DocumentInvalid)?, + resolution: ConflictResolution::parse(resolution) + .map_err(|_| ReviewCustodyError::DocumentInvalid)?, + }, + Self::MergePullRequest { + pull_request_id, + expected_pull_request_revision, + expected_head_revision, + } => ReviewAction::MergePullRequest { + pull_request_id: PullRequestId::new(pull_request_id.clone()) + .map_err(|_| ReviewCustodyError::DocumentInvalid)?, + expected_pull_request_revision: Revision::new(*expected_pull_request_revision) + .map_err(|_| ReviewCustodyError::DocumentInvalid)?, + expected_head_revision: ReviewField::new(expected_head_revision.clone()) + .map_err(|_| ReviewCustodyError::DocumentInvalid)?, + }, }) } } +/// Re-parse a persisted proposal id through the protocol's own grammar. +/// +/// The looser of two spellings is the one a recovered plan replays through, so +/// a document this process has not yet trusted is never allowed to widen what +/// a proposal id may be. +fn disk_proposal_id(value: &str) -> Result { + ReviewProposalId::new(value.to_owned()).map_err(|_| ReviewCustodyError::DocumentInvalid) +} + #[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)] #[serde(deny_unknown_fields)] struct ReviewConfirmationDisk { @@ -1029,6 +1143,7 @@ mod tests { ) .unwrap()], ReviewPullRequestCapabilities::default(), + ReviewGitStagingCapabilities::default(), ) .unwrap(); PlatformReviewActionPreview::batch_send_comments( @@ -1103,6 +1218,144 @@ mod tests { ); } + /// One confirmed staging preview, built from the advertisement a coherent + /// server would mint for this snapshot's `Commit` proposal. + fn staging_preview() -> PlatformReviewActionPreview { + let review = semantic(); + let target = target(&review); + let proposal = &review.proposals[0]; + let capabilities = ReviewCapabilities::new( + target.project.clone(), + target.workspace.clone(), + review.revision, + Revision::new(91).unwrap(), + Vec::new(), + Vec::new(), + ReviewPullRequestCapabilities::default(), + ReviewGitStagingCapabilities { + staging: vec![ReviewStagingCapability::new( + ReviewProposalId::new(proposal.id.clone()).unwrap(), + proposal.kind, + ReviewField::new("7".repeat(40)).unwrap(), + ReviewIndexDigest::new("a".repeat(64)).unwrap(), + ReviewAuthority::new( + ReviewAuthorityKind::Git, + automonique_protocol::platform_v2_review::ReviewAuthorityId::new( + proposal.authority.as_ref().unwrap().id.clone(), + ) + .unwrap(), + ), + ReviewConfirmationDigest::new("c".repeat(64)).unwrap(), + ReviewReceiptCorrelationDigest::new("d".repeat(64)).unwrap(), + ) + .unwrap()], + conflict_resolutions: Vec::new(), + }, + ) + .unwrap(); + PlatformReviewActionPreview::stage_proposal(target, &review, &capabilities, &proposal.id) + .unwrap() + } + + // SDTEST-1870 — the confirmed staging families take the same durable + // at-most-once lane as every other exposed mutation, and survive the round + // trip through disk with their exact coordinates and digests intact. + // + // The disk format did not know these actions before this slice, exactly as + // it did not know the batch delivery before PR #161: `prepare` would have + // returned `UnsupportedAction` and the dispatch would have died at the + // fence. That assumption was wrong once already, which is why this test + // drives the whole store rather than only the enum. + #[test] + fn confirmed_staging_is_fenced_and_survives_the_disk_round_trip() { + let directory = TempRoot::new(); + let preview = staging_preview(); + // Staging is a confirmed lane now: the unconfirmed spelling cannot be + // constructed at all, so a record without a confirmation would be an + // unsendable document. + let confirmation = preview.confirmation().expect("staging carries a digest"); + assert_eq!(confirmation.confirmation_digest().as_str(), "c".repeat(64)); + assert_eq!( + confirmation.receipt_correlation_digest().as_str(), + "d".repeat(64) + ); + assert!(preview.requires_capability_revalidation()); + + let store = PlatformReviewCustodyStore::open(store_path(&directory)).unwrap(); + store.prepare(&preview).unwrap(); + assert!(matches!( + store.prepare(&staging_preview()), + Err(ReviewCustodyError::EffectAlreadyPending) + )); + + // A restart before dispatch reports it as never started, and the exact + // action plus both digests survive serialization. + 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-write must look the receipt up through its correlation, never re-post" + ); + } + + // SDTEST-1871 — every confirmed staging family round-trips through the + // disk enum, and the side of a conflict resolution is part of the persisted + // identity rather than a value recomputed on recovery. + #[test] + fn every_staging_family_round_trips_and_a_resolution_keeps_its_side() { + for action in [ + ReviewAction::Stage { + proposal_id: ReviewProposalId::new("proposal-1".to_owned()).unwrap(), + }, + ReviewAction::Unstage { + proposal_id: ReviewProposalId::new("proposal-1".to_owned()).unwrap(), + }, + ReviewAction::Commit { + proposal_id: ReviewProposalId::new("proposal-1".to_owned()).unwrap(), + }, + ReviewAction::ResolveConflict { + proposal_id: ReviewProposalId::new("proposal-1".to_owned()).unwrap(), + file_id: ReviewFileId::new("file-1".to_owned()).unwrap(), + resolution: ConflictResolution::KeepCurrent, + }, + ReviewAction::ResolveConflict { + proposal_id: ReviewProposalId::new("proposal-1".to_owned()).unwrap(), + file_id: ReviewFileId::new("file-1".to_owned()).unwrap(), + resolution: ConflictResolution::KeepIncoming, + }, + ] { + let disk = ReviewActionDisk::from_action(&action).expect("family is spelled on disk"); + let json = serde_json::to_string(&disk).expect("serializable"); + let decoded: ReviewActionDisk = serde_json::from_str(&json).expect("decodable"); + assert_eq!( + decoded.to_action().expect("re-parses through the grammar"), + action, + "a persisted staging action must replay as exactly itself" + ); + } + + // A document whose confirmation no longer matches its family cannot be + // replayed onto the wire, so it is rejected as invalid rather than + // recovered into an unsendable preview. + let mut record = + ReviewCustodyRecord::from_preview(&staging_preview(), ReviewCustodyState::Prepared) + .unwrap(); + record.confirmation = None; + assert!(matches!( + record.to_preview(), + Err(ReviewCustodyError::DocumentInvalid) + )); + } + #[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 index 655f8def..bf93c297 100644 --- a/crates/shelldeck-core/src/config/platform_review/delivery.rs +++ b/crates/shelldeck-core/src/config/platform_review/delivery.rs @@ -245,7 +245,8 @@ impl ReviewAgentDeliveryProjection { mod tests { use super::super::{ CommentAgentState, PlatformReviewSemantic, ReviewAgentDeliveryCapability, ReviewAuthority, - ReviewCommentId, ReviewFreshnessState, ReviewPullRequestCapabilities, + ReviewCommentId, ReviewFreshnessState, ReviewGitStagingCapabilities, + ReviewPullRequestCapabilities, }; use super::*; use automonique_protocol::platform_v2::{ProjectId, WorkContextIdentity}; @@ -296,6 +297,7 @@ mod tests { Vec::new(), advertised, ReviewPullRequestCapabilities::default(), + ReviewGitStagingCapabilities::default(), ) .unwrap() } diff --git a/crates/shelldeck-core/src/config/platform_review/worktree.rs b/crates/shelldeck-core/src/config/platform_review/worktree.rs index 8d0d4ff1..79ba257e 100644 --- a/crates/shelldeck-core/src/config/platform_review/worktree.rs +++ b/crates/shelldeck-core/src/config/platform_review/worktree.rs @@ -5,15 +5,44 @@ //! staged/unstaged/untracked/conflicted lane every observed file belongs to, //! and what — if anything — may be painted for that file's preview. //! -//! It grants no authority. Staging is a Git mutation, so the projection -//! reports the server's *proposed* transitions as observations and separately -//! reports why no staging control may be offered. See -//! [`advertised_staging_capability`] for the exact contract gap. +//! It grants no authority of its own. Staging is a Git mutation, so a control +//! exists only where the server advertised a slot for that exact proposal, and +//! the projection otherwise reports which fence is missing so the surface can +//! say so instead of offering a control that would refuse. +//! +//! # What the server has to prove before a control exists +//! +//! A worktree is shared substrate: any other process running as the daemon uid +//! can move `HEAD` and rewrite the index between an advertisement and the +//! action arriving, and the snapshot revision does not help — it tracks what +//! the projection observed, not what the repository now is. So every entry in +//! `staging` and `conflict_resolutions` names both mutable things the write +//! depends on, and its confirmation digest commits to them. This module +//! carries that observation verbatim ([`ReviewWorktreeObservation`]) so a +//! reader can see what the server read rather than having to trust that it +//! read anything. +//! +//! # Absence is the withholding +//! +//! The three staging kinds share one capability type but not one grant: an +//! operator withholds committing separately from index writes, and conflict +//! resolution separately again. A deployment that grants stage and unstage but +//! not commit therefore produces a `staging` list with `Stage` and `Unstage` +//! entries and no `Commit` entry — and the commit control must simply not +//! exist. A disabled button with a tooltip would claim the grant is present +//! and merely unavailable, which is the opposite of what the list says. +//! +//! # No hunk granularity +//! +//! [`super::ReviewProposalId`] names a proposal and a proposal lists file ids; +//! `ReviewAnchor`'s hunk id exists for comments only. File-level staging is +//! all the contract can express, so it is all this module offers. A hunk +//! control here would be advertising something no action can name. use super::{ - ConflictState, DiffChangeKind, DiffSide, PlatformReviewSemantic, PreviewKind, - ReviewAnchorSemantic, ReviewCapabilities, ReviewFileSemantic, ReviewProposalKind, - WorktreeFileState, + ConflictResolution, ConflictState, DiffChangeKind, DiffSide, PlatformReviewSemantic, + PlatformReviewTarget, PreviewKind, ReviewAnchorSemantic, ReviewAuthorityKind, + ReviewCapabilities, ReviewFileSemantic, ReviewProposalKind, WorktreeFileState, }; use automonique_protocol::primitives::Revision; @@ -98,21 +127,32 @@ impl ReviewWorktreeLane { /// Why the projection offers no Git staging control. #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum ReviewStagingWithheld { - /// The separately fetched capability response advertises no staging - /// capability for this exact project, workspace, and snapshot revision. + /// 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 both its git lists are empty. + /// That is the server's honest fail-closed answer — no registry binding + /// granting index writes, a repository the preflight could not read, an + /// unborn `HEAD`, or a worktree in a state no staging write can be fenced + /// against — and it must produce no control. + NoStageableProposal, /// No durable at-most-once custody store is available in this process. NoCustodyLane, } impl ReviewStagingWithheld { /// Every reason a surface must be able to explain. - pub const ALL: [Self; 2] = [Self::NoServerCapability, Self::NoCustodyLane]; + pub const ALL: [Self; 3] = [ + Self::NoServerCapability, + Self::NoStageableProposal, + Self::NoCustodyLane, + ]; #[must_use] pub const fn as_str(self) -> &'static str { match self { Self::NoServerCapability => "no_server_capability", + Self::NoStageableProposal => "no_stageable_proposal", Self::NoCustodyLane => "no_custody_lane", } } @@ -123,38 +163,187 @@ impl ReviewStagingWithheld { } } -/// Whether the server advertised an exact Git staging capability. +/// The one repository read every git capability in a response was minted from. +/// +/// `ReviewCapabilities::new` refuses a response whose git entries disagree on +/// either field, so this is a property of the whole document rather than of an +/// entry. It is carried here for the reason the server carries it at all: a +/// client must be able to see the `HEAD` and index the preflight observed, and +/// decline to offer a control once it holds a document that read a different +/// worktree. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct ReviewWorktreeObservation { + /// The commit `HEAD` resolved to when the server read the repository. + pub head_revision: String, + /// A digest over the whole index as the preflight read it. + pub index_digest: String, +} + +/// One staging transition the server proved it can perform, carried verbatim. +/// +/// The two confirmation digests are deliberately absent. They are minted per +/// entry and must cross the wire exactly as the server spelled them, so +/// [`super::PlatformReviewActionPreview`] reads them straight off the +/// capability rather than through a render-side copy that could drift. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct AdvertisedReviewStaging { + pub proposal_id: String, + pub kind: ReviewProposalKind, + pub expected_head_revision: String, + pub expected_index_digest: String, + pub authority_id: String, +} + +/// One conflicted file the server proved it can collapse to one recorded side. +/// +/// The server lists **one entry per admissible side**, not one per file with +/// the side left to the client: the side decides which bytes land, so it is +/// inside the confirmation digest, and a digest cannot commit to a choice not +/// yet made. A file with both stage 2 and stage 3 recorded therefore yields +/// two entries here, and the surface renders two controls. A delete/modify +/// conflict has one side recorded and yields one. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct AdvertisedReviewConflictResolution { + pub proposal_id: String, + pub file_id: String, + pub resolution: ConflictResolution, + pub expected_head_revision: String, + pub expected_index_digest: String, + pub authority_id: String, +} + +/// Whether a capability response is attributed to this exact review read. +fn exact_capability_attribution( + capabilities: &ReviewCapabilities, + target: &PlatformReviewTarget, + review: &PlatformReviewSemantic, +) -> bool { + 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() +} + +/// The exact advertised staging set for one active snapshot. +/// +/// Every entry survives three checks, exactly like the delivery lane: the +/// capability response is attributed to this project, workspace and snapshot +/// revision; its authority is a Git authority the proposal itself names; and +/// the proposal is present in that same snapshot, at the same kind, and +/// locally actionable. /// -/// [`ReviewCapabilities`] carries `rerunnable_checks` and nothing else. The -/// Platform v2 contract defines no staging capability, no staging confirmation -/// digest, and no staging receipt-correlation digest, and -/// [`super::PlatformReviewActionPreview`] structurally refuses a confirmation -/// on `Stage`, `Unstage`, `Commit`, and `ResolveConflict`. There is therefore -/// nothing a client could examine that would prove staging authority, and a -/// proposal's `git` authority inside the read snapshot is an observation, not -/// a capability. This stays `false` for every capability load until the -/// contract grows the missing lane. +/// The third check is a torn-read guard, not a second source of authority. The +/// capability list and the snapshot are two separate reads; a disagreement +/// means they straddled a change, and refusing the entry is the fail-closed +/// answer. It can never *add* an entry the server did not advertise, which is +/// what makes a withheld `commit` grant render as absence. #[must_use] -pub const fn advertised_staging_capability(_capabilities: Option<&ReviewCapabilities>) -> bool { - false +pub fn advertised_staging_proposals( + capabilities: Option<&ReviewCapabilities>, + target: &PlatformReviewTarget, + review: &PlatformReviewSemantic, +) -> Vec { + let Some(capabilities) = capabilities else { + return Vec::new(); + }; + if !exact_capability_attribution(capabilities, target, review) { + return Vec::new(); + } + capabilities + .staging() + .iter() + .filter(|capability| capability.authority().kind() == ReviewAuthorityKind::Git) + .filter(|capability| { + review.proposals.iter().any(|proposal| { + proposal.id == capability.proposal_id().as_str() + && proposal.kind == capability.kind() + && proposal.authority.as_ref().is_some_and(|authority| { + authority.kind == ReviewAuthorityKind::Git + && authority.id == capability.authority().id().as_str() + }) + }) && review.proposal_is_actionable(capability.proposal_id().as_str()) + }) + .map(|capability| AdvertisedReviewStaging { + proposal_id: capability.proposal_id().as_str().to_owned(), + kind: capability.kind(), + expected_head_revision: capability.expected_head_revision().as_str().to_owned(), + expected_index_digest: capability.expected_index_digest().as_str().to_owned(), + authority_id: capability.authority().id().as_str().to_owned(), + }) + .collect() } -/// Decide whether a Git staging control may be rendered at all. +/// The exact advertised conflict-resolution set for one active snapshot. /// -/// Both fences must hold, exactly like the confirmed check rerun: the server -/// must advertise the capability, and the durable custody store must be able -/// to record the at-most-once boundary before dispatch. A missing fence makes -/// the control absent, never optimistically disabled. +/// The snapshot-side guard differs from the staging one because the domain +/// does: the proposal must be a `ResolveConflict` on a Git authority, the file +/// must be one the proposal names, and that file must still be unmerged in +/// this snapshot. A file git no longer reports as conflicted cannot be +/// collapsed to a side, whatever an older capability read said. +#[must_use] +pub fn advertised_conflict_resolutions( + capabilities: Option<&ReviewCapabilities>, + target: &PlatformReviewTarget, + review: &PlatformReviewSemantic, +) -> Vec { + let Some(capabilities) = capabilities else { + return Vec::new(); + }; + if !exact_capability_attribution(capabilities, target, review) { + return Vec::new(); + } + capabilities + .conflict_resolutions() + .iter() + .filter(|capability| capability.authority().kind() == ReviewAuthorityKind::Git) + .filter(|capability| { + review.conflict_resolution_is_actionable( + capability.proposal_id().as_str(), + capability.file_id().as_str(), + ) && review.proposals.iter().any(|proposal| { + proposal.id == capability.proposal_id().as_str() + && proposal.authority.as_ref().is_some_and(|authority| { + authority.id == capability.authority().id().as_str() + }) + }) + }) + .map(|capability| AdvertisedReviewConflictResolution { + proposal_id: capability.proposal_id().as_str().to_owned(), + file_id: capability.file_id().as_str().to_owned(), + resolution: capability.resolution(), + expected_head_revision: capability.expected_head_revision().as_str().to_owned(), + expected_index_digest: capability.expected_index_digest().as_str().to_owned(), + authority_id: capability.authority().id().as_str().to_owned(), + }) + .collect() +} + +/// Decide whether any Git staging control may be rendered at all. +/// +/// Three fences must hold, exactly like the batch delivery lane: a capability +/// response attributed to this exact snapshot, at least one entry inside it, +/// and a durable custody store able to record the at-most-once boundary before +/// dispatch. A missing fence makes the control absent, never optimistically +/// disabled. +/// +/// This is the lane-level answer. It says a control *may* exist, never that +/// any particular one does: each proposal still needs its own advertised slot, +/// which is how a deployment withholding `commit` while granting `stage` and +/// `unstage` produces two controls and not three. /// /// # Errors /// /// Returns the first unmet fence. pub const fn review_staging_control( - server_capability: bool, + exact_capability: bool, + advertised: bool, custody_available: bool, ) -> Result<(), ReviewStagingWithheld> { - if !server_capability { + if !exact_capability { Err(ReviewStagingWithheld::NoServerCapability) + } else if !advertised { + Err(ReviewStagingWithheld::NoStageableProposal) } else if !custody_available { Err(ReviewStagingWithheld::NoCustodyLane) } else { @@ -313,10 +502,13 @@ pub struct ReviewWorktreeLaneGroup { pub struct ReviewWorktreeProjection { pub source_revision: Revision, pub lanes: Vec, - /// `Ok` only when a real server capability and a durable custody lane both - /// back a staging mutation. Platform v2 advertises none, so this reports - /// the exact missing fence instead of a control. + /// `Ok` only when an exact server capability, at least one advertised + /// entry, and a durable custody lane all back a staging mutation. + /// Otherwise it names the exact missing fence instead of a control. pub staging: Result<(), ReviewStagingWithheld>, + advertised_staging: Vec, + advertised_conflict_resolutions: Vec, + observation: Option, } impl ReviewWorktreeProjection { @@ -324,16 +516,53 @@ impl ReviewWorktreeProjection { /// /// `custody_available` is the caller's durable at-most-once store; it is /// never inferred from the snapshot. + /// `target` is optional because the lanes are not: a surface with no + /// exactly reconciled target still renders the combined read, it simply + /// has nothing a capability response could be attributed to, so every + /// control is withheld for want of a server capability. #[must_use] pub fn new( review: &PlatformReviewSemantic, + target: Option<&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. Two + // empty git lists inside it is a different, more specific answer, and + // the surface must be able to say which one it got. + let exact_capability = target + .zip(capabilities) + .is_some_and(|(target, capabilities)| { + exact_capability_attribution(capabilities, target, review) + }); + let advertised_staging = target.map_or_else(Vec::new, |target| { + advertised_staging_proposals(capabilities, target, review) + }); + let advertised_conflict_resolutions = target.map_or_else(Vec::new, |target| { + advertised_conflict_resolutions(capabilities, target, review) + }); let staging = review_staging_control( - advertised_staging_capability(capabilities), + exact_capability, + !advertised_staging.is_empty() || !advertised_conflict_resolutions.is_empty(), custody_available, ); + // Every git entry in one response shares one repository read, so the + // first admitted entry carries the whole document's observation. + let observation = advertised_staging + .first() + .map(|entry| ReviewWorktreeObservation { + head_revision: entry.expected_head_revision.clone(), + index_digest: entry.expected_index_digest.clone(), + }) + .or_else(|| { + advertised_conflict_resolutions + .first() + .map(|entry| ReviewWorktreeObservation { + head_revision: entry.expected_head_revision.clone(), + index_digest: entry.expected_index_digest.clone(), + }) + }); let lanes = ReviewWorktreeLane::ALL .into_iter() .map(|lane| ReviewWorktreeLaneGroup { @@ -351,6 +580,9 @@ impl ReviewWorktreeProjection { source_revision: review.revision, lanes, staging, + advertised_staging, + advertised_conflict_resolutions, + observation, } } @@ -367,6 +599,49 @@ impl ReviewWorktreeProjection { ids.dedup(); ids.len() } + + /// The advertised staging set, in the capability's own order. + #[must_use] + pub fn advertised_staging(&self) -> &[AdvertisedReviewStaging] { + &self.advertised_staging + } + + /// The advertised conflict-resolution set, in the capability's own order. + #[must_use] + pub fn advertised_conflict_resolutions(&self) -> &[AdvertisedReviewConflictResolution] { + &self.advertised_conflict_resolutions + } + + /// The `HEAD` and index the server read, when it advertised anything. + #[must_use] + pub const fn observation(&self) -> Option<&ReviewWorktreeObservation> { + self.observation.as_ref() + } + + /// Whether this exact proposal may carry a staging control. + /// + /// This is the whole of the withholding rule at the render site: a + /// `Commit` proposal the server did not advertise answers `false` here and + /// simply grows no button, while its `Stage` and `Unstage` siblings do. + #[must_use] + pub fn advertises_staging(&self, proposal_id: &str) -> bool { + self.advertised_staging + .iter() + .any(|entry| entry.proposal_id == proposal_id) + } + + /// Every admissible side for one conflicted file, in advertisement order. + /// + /// The server lists one entry per side it actually recorded, so this + /// returns two for a file holding both and one for a delete/modify pair. + #[must_use] + pub fn advertised_sides(&self, proposal_id: &str, file_id: &str) -> Vec { + self.advertised_conflict_resolutions + .iter() + .filter(|entry| entry.proposal_id == proposal_id && entry.file_id == file_id) + .map(|entry| entry.resolution) + .collect() + } } /// The lanes one file belongs to, in presentation order. @@ -594,14 +869,19 @@ mod tests { use crate::config::platform_review::{ AttentionState, DeliverySemantic, DeliveryState, MergeReadiness, PlatformReviewActionPreview, PlatformReviewTarget, PullRequestSemantic, PullRequestState, - ReviewAttentionSemantic, ReviewAuthorityKind, ReviewAuthoritySemantic, ReviewDecision, - ReviewFreshnessSemantic, ReviewFreshnessState, ReviewHunkSemantic, ReviewPreviewSemantic, - ReviewProposalSemantic, ReviewSchemaVersion, ReviewStatusSemantic, + ReviewAction, ReviewAttentionSemantic, ReviewAuthority, ReviewAuthorityKind, + ReviewAuthoritySemantic, ReviewConfirmationDigest, ReviewConflictResolutionCapability, + ReviewDecision, ReviewField, ReviewFreshnessSemantic, ReviewFreshnessState, + ReviewGitStagingCapabilities, ReviewHunkSemantic, ReviewIndexDigest, ReviewPreviewSemantic, + ReviewProposalId, ReviewProposalSemantic, ReviewPullRequestCapabilities, + ReviewReceiptCorrelationDigest, ReviewSchemaVersion, ReviewStagingCapability, + ReviewStatusSemantic, }; use crate::config::workspace_catalog::{ PlatformContextRef, PlatformMappingReconciliation, PlatformV2Mapping, }; use automonique_protocol::platform_v2::WorkContextTargetKind; + use automonique_protocol::platform_v2_review::{ReviewAuthorityId, ReviewFileId}; use automonique_protocol::platform_v2_review_api::decode_review_snapshot; const CANONICAL_FIXTURE: &[u8] = @@ -772,7 +1052,7 @@ mod tests { ], Vec::new(), ); - let projection = ReviewWorktreeProjection::new(&review, None, true); + let projection = ReviewWorktreeProjection::new(&review, None, None, true); assert_eq!( projection @@ -837,7 +1117,7 @@ mod tests { ], Vec::new(), ); - let projection = ReviewWorktreeProjection::new(&review, None, true); + let projection = ReviewWorktreeProjection::new(&review, None, None, true); let anchors = projection .lanes .iter() @@ -1037,16 +1317,102 @@ mod tests { ); } - // SDTEST-1848 — a snapshot proposal carrying a `git` authority is an - // observation. Platform v2 advertises no staging capability, so the - // projection reports the missing fence and never an offerable control. - #[test] - fn sdtest_1848_staging_stays_withheld_for_every_capability_load() { - let review = review_with( + const OBSERVED_HEAD: &str = "1f0e4b8c2a6d9e3f7051c8b4a2d6e9f30517b2c4"; + const OTHER_HEAD: &str = "9a8b7c6d5e4f30219a8b7c6d5e4f30219a8b7c6d"; + + fn index_digest(seed: char) -> ReviewIndexDigest { + ReviewIndexDigest::new(seed.to_string().repeat(64)).expect("index digest") + } + + /// Two distinct lowercase-hex digests per seed, so a test can assert that + /// the confirmation and the correlation are carried separately rather than + /// one being reused for both. + fn digests(seed: char) -> (ReviewConfirmationDigest, ReviewReceiptCorrelationDigest) { + let confirmation = format!("{}{}", seed.to_string().repeat(63), '0'); + let correlation = format!("{}{}", seed.to_string().repeat(63), '1'); + ( + ReviewConfirmationDigest::new(confirmation).expect("confirmation"), + ReviewReceiptCorrelationDigest::new(correlation).expect("correlation"), + ) + } + + fn git_authority() -> ReviewAuthority { + ReviewAuthority::new( + ReviewAuthorityKind::Git, + ReviewAuthorityId::new("authority-1".to_owned()).expect("authority id"), + ) + } + + fn staging_capability( + proposal_id: &str, + kind: ReviewProposalKind, + head: &str, + index: char, + seed: char, + ) -> ReviewStagingCapability { + let (confirmation, correlation) = digests(seed); + ReviewStagingCapability::new( + ReviewProposalId::new(proposal_id.to_owned()).expect("proposal id"), + kind, + ReviewField::new(head.to_owned()).expect("head revision"), + index_digest(index), + git_authority(), + confirmation, + correlation, + ) + .expect("staging capability") + } + + fn conflict_capability( + proposal_id: &str, + file_id: &str, + resolution: ConflictResolution, + index: char, + seed: char, + ) -> ReviewConflictResolutionCapability { + let (confirmation, correlation) = digests(seed); + ReviewConflictResolutionCapability::new( + ReviewProposalId::new(proposal_id.to_owned()).expect("proposal id"), + ReviewFileId::new(file_id.to_owned()).expect("file id"), + resolution, + ReviewField::new(OBSERVED_HEAD.to_owned()).expect("head revision"), + index_digest(index), + git_authority(), + confirmation, + correlation, + ) + .expect("conflict capability") + } + + fn capabilities( + target: &PlatformReviewTarget, + review: &PlatformReviewSemantic, + git: ReviewGitStagingCapabilities, + ) -> ReviewCapabilities { + ReviewCapabilities::new( + target.project.clone(), + target.workspace.clone(), + review.revision, + revision(91), + Vec::new(), + Vec::new(), + ReviewPullRequestCapabilities::default(), + git, + ) + .expect("capability response") + } + + /// One unstaged file with a `Stage` proposal, one with an `Unstage`, one + /// with a `Commit`, and one unmerged file carrying a `ResolveConflict` + /// proposal over both sides. + fn staging_review() -> PlatformReviewSemantic { + review_with( vec![ text_file("file-1", WorktreeFileState::Unstaged, ConflictState::None), + text_file("file-2", WorktreeFileState::Staged, ConflictState::None), + text_file("file-3", WorktreeFileState::Staged, ConflictState::None), text_file( - "file-2", + "file-conflict", WorktreeFileState::Unstaged, ConflictState::Unresolved, ), @@ -1060,47 +1426,410 @@ mod tests { subject: None, }, ReviewProposalSemantic { - id: "proposal-blocked".to_owned(), - kind: ReviewProposalKind::Stage, + id: "proposal-unstage".to_owned(), + kind: ReviewProposalKind::Unstage, authority: Some(authority(ReviewAuthorityKind::Git)), files: vec!["file-2".to_owned()], subject: None, }, + ReviewProposalSemantic { + id: "proposal-commit".to_owned(), + kind: ReviewProposalKind::Commit, + authority: Some(authority(ReviewAuthorityKind::Git)), + files: vec!["file-3".to_owned()], + subject: Some("record the reviewed change".to_owned()), + }, + ReviewProposalSemantic { + id: "proposal-conflict".to_owned(), + kind: ReviewProposalKind::ResolveConflict, + authority: Some(authority(ReviewAuthorityKind::Git)), + files: vec!["file-conflict".to_owned()], + subject: None, + }, ], + ) + } + + // SDTEST-1867 — the withholding demonstration. A deployment that installs + // `index_write` but not `commit` advertises `Stage` and `Unstage` and no + // `Commit`. The commit control must be *absent*, not disabled: the three + // kinds share one capability type but not one grant, so the only honest + // rendering of a missing grant is a missing control. The two granted kinds + // must go all the way through to a dispatchable confirmed preview in the + // same run, or the test would prove nothing about the ones that are there. + #[test] + fn sdtest_1867_withheld_commit_grant_renders_no_control_while_its_siblings_go_end_to_end() { + let review = staging_review(); + let target = target(); + let granted = capabilities( + &target, + &review, + ReviewGitStagingCapabilities { + staging: vec![ + staging_capability( + "proposal-stage", + ReviewProposalKind::Stage, + OBSERVED_HEAD, + 'a', + 'c', + ), + staging_capability( + "proposal-unstage", + ReviewProposalKind::Unstage, + OBSERVED_HEAD, + 'a', + 'd', + ), + ], + conflict_resolutions: Vec::new(), + }, ); - assert!(!advertised_staging_capability(None)); + let projection = + ReviewWorktreeProjection::new(&review, Some(&target), Some(&granted), true); + assert_eq!(projection.staging, Ok(())); assert_eq!( - ReviewWorktreeProjection::new(&review, None, true).staging, + projection + .advertised_staging() + .iter() + .map(|entry| (entry.proposal_id.as_str(), entry.kind)) + .collect::>(), + [ + ("proposal-stage", ReviewProposalKind::Stage), + ("proposal-unstage", ReviewProposalKind::Unstage), + ] + ); + assert!( + !projection.advertises_staging("proposal-commit"), + "a grant the server withheld must produce no control at all" + ); + // The commit proposal is still *observed* — the read side never lost + // it — which is exactly what distinguishes absence-of-control from + // absence-of-proposal. + let staged = lane(&projection, ReviewWorktreeLane::Staged); + assert!(staged.iter().any(|file| file + .staging + .iter() + .any(|proposal| proposal.proposal_id == "proposal-commit" && proposal.admissible))); + + // The withheld kind refuses at the preview constructor too, so nothing + // downstream can reconstruct it from the snapshot's own proposal. + assert!(PlatformReviewActionPreview::stage_proposal( + target.clone(), + &review, + &granted, + "proposal-commit", + ) + .is_err()); + + // The two granted kinds go end to end: a confirmed preview carrying the + // server's own digests verbatim, which the fresh capability document + // still admits. + for (proposal_id, seed) in [("proposal-stage", 'c'), ("proposal-unstage", 'd')] { + let preview = PlatformReviewActionPreview::stage_proposal( + target.clone(), + &review, + &granted, + proposal_id, + ) + .expect("granted staging proposal is dispatchable"); + let (confirmation, correlation) = digests(seed); + let carried = preview.confirmation().expect("staging is a confirmed lane"); + assert_eq!(carried.confirmation_digest(), &confirmation); + assert_eq!(carried.receipt_correlation_digest(), &correlation); + assert_eq!(carried.expected_workspace_revision(), revision(91)); + assert!(preview.requires_capability_revalidation()); + assert!(preview.matches_capabilities(&granted)); + } + + // Grant the commit too and the third control appears, from the same + // projection code and with no other change. + let full = capabilities( + &target, + &review, + ReviewGitStagingCapabilities { + staging: vec![ + staging_capability( + "proposal-stage", + ReviewProposalKind::Stage, + OBSERVED_HEAD, + 'a', + 'c', + ), + staging_capability( + "proposal-unstage", + ReviewProposalKind::Unstage, + OBSERVED_HEAD, + 'a', + 'd', + ), + staging_capability( + "proposal-commit", + ReviewProposalKind::Commit, + OBSERVED_HEAD, + 'a', + 'e', + ), + ], + conflict_resolutions: Vec::new(), + }, + ); + let projection = ReviewWorktreeProjection::new(&review, Some(&target), Some(&full), true); + assert!(projection.advertises_staging("proposal-commit")); + assert!(PlatformReviewActionPreview::stage_proposal( + target.clone(), + &review, + &full, + "proposal-commit", + ) + .is_ok()); + } + + // SDTEST-1868 — every other way the lane can be withheld, and the fence + // that a worktree moving under the daemon has to trip. + #[test] + fn sdtest_1868_staging_lane_names_its_missing_fence_and_refuses_a_moved_worktree() { + let review = staging_review(); + let target = target(); + let granted = capabilities( + &target, + &review, + ReviewGitStagingCapabilities { + staging: vec![staging_capability( + "proposal-stage", + ReviewProposalKind::Stage, + OBSERVED_HEAD, + 'a', + 'c', + )], + conflict_resolutions: Vec::new(), + }, + ); + + assert_eq!( + ReviewWorktreeProjection::new(&review, Some(&target), None, true).staging, Err(ReviewStagingWithheld::NoServerCapability) ); assert_eq!( - ReviewWorktreeProjection::new(&review, None, false).staging, + ReviewWorktreeProjection::new(&review, None, Some(&granted), true).staging, Err(ReviewStagingWithheld::NoServerCapability), - "the absent server capability is reported before the custody lane" + "no exactly reconciled target means nothing can be attributed" + ); + // An exact response with two empty git lists is a different, more + // specific answer than no response at all, and the surface must be + // able to say which one it got. + let empty = capabilities(&target, &review, ReviewGitStagingCapabilities::default()); + assert_eq!( + ReviewWorktreeProjection::new(&review, Some(&target), Some(&empty), true).staging, + Err(ReviewStagingWithheld::NoStageableProposal) ); assert_eq!( - review_staging_control(true, false), - Err(ReviewStagingWithheld::NoCustodyLane) + ReviewWorktreeProjection::new(&review, Some(&target), Some(&granted), false).staging, + Err(ReviewStagingWithheld::NoCustodyLane), + "the durable lane is reported last, once the server has proved a slot" ); - assert_eq!(review_staging_control(true, true), Ok(())); + // The lanes render in every one of those states; only the control is + // withheld. + for projection in [ + ReviewWorktreeProjection::new(&review, Some(&target), None, true), + ReviewWorktreeProjection::new(&review, Some(&target), Some(&empty), true), + ] { + assert_eq!(projection.distinct_file_count(), 4); + assert!(projection.advertised_staging().is_empty()); + assert!(projection.observation().is_none()); + } + + // The observation is exposed so a reader can see the fence. + let projection = + ReviewWorktreeProjection::new(&review, Some(&target), Some(&granted), true); + let observation = projection + .observation() + .expect("granted lane names its read"); + assert_eq!(observation.head_revision, OBSERVED_HEAD); + assert_eq!(observation.index_digest, "a".repeat(64)); + + // A preview minted against one worktree read must stop matching once a + // fresher document reports a different `HEAD` or a different index. + // The server mints the digest over both, so the comparison is the + // digest — and the refusal is local, before anything reaches the + // daemon. + let preview = PlatformReviewActionPreview::stage_proposal( + target.clone(), + &review, + &granted, + "proposal-stage", + ) + .expect("advertised proposal"); + for moved in [ + staging_capability( + "proposal-stage", + ReviewProposalKind::Stage, + OTHER_HEAD, + 'a', + 'f', + ), + staging_capability( + "proposal-stage", + ReviewProposalKind::Stage, + OBSERVED_HEAD, + 'b', + 'f', + ), + ] { + let refreshed = capabilities( + &target, + &review, + ReviewGitStagingCapabilities { + staging: vec![moved], + conflict_resolutions: Vec::new(), + }, + ); + assert!( + !preview.matches_capabilities(&refreshed), + "a worktree that moved must withdraw the control locally" + ); + } - let projection = ReviewWorktreeProjection::new(&review, None, true); - let unstaged = lane(&projection, ReviewWorktreeLane::Unstaged); - assert_eq!(unstaged.len(), 1); + // A capability whose kind disagrees with the snapshot's proposal is a + // torn read between two server-side reads, and is refused rather than + // letting the client pick which one to believe. + let torn = capabilities( + &target, + &review, + ReviewGitStagingCapabilities { + staging: vec![staging_capability( + "proposal-stage", + ReviewProposalKind::Commit, + OBSERVED_HEAD, + 'a', + 'c', + )], + conflict_resolutions: Vec::new(), + }, + ); + assert!( + ReviewWorktreeProjection::new(&review, Some(&target), Some(&torn), true) + .advertised_staging() + .is_empty() + ); + } + + // SDTEST-1869 — `conflict_resolutions` lists one entry per admissible + // side, so a file with both sides recorded yields two controls and a + // delete/modify pair yields one. The side is part of the identity, not a + // parameter chosen after the fact: each entry carries the digest minted + // over the blob that side would write. + #[test] + fn sdtest_1869_both_recorded_sides_render_two_controls_and_a_single_side_renders_one() { + let review = staging_review(); + let target = target(); + let both = capabilities( + &target, + &review, + ReviewGitStagingCapabilities { + staging: Vec::new(), + conflict_resolutions: vec![ + conflict_capability( + "proposal-conflict", + "file-conflict", + ConflictResolution::KeepCurrent, + 'a', + 'c', + ), + conflict_capability( + "proposal-conflict", + "file-conflict", + ConflictResolution::KeepIncoming, + 'a', + 'd', + ), + ], + }, + ); + let projection = ReviewWorktreeProjection::new(&review, Some(&target), Some(&both), true); + assert_eq!(projection.staging, Ok(())); assert_eq!( - unstaged[0].staging, - vec![ReviewStagingProposal { - proposal_id: "proposal-stage".to_owned(), - kind: ReviewProposalKind::Stage, - admissible: true, - }] + projection.advertised_sides("proposal-conflict", "file-conflict"), + [ + ConflictResolution::KeepCurrent, + ConflictResolution::KeepIncoming + ], + "one entry per admissible side means two buttons, not one with a choice" ); - let conflicted = lane(&projection, ReviewWorktreeLane::Conflicted); - assert_eq!(conflicted.len(), 1); + + for (resolution, seed) in [ + (ConflictResolution::KeepCurrent, 'c'), + (ConflictResolution::KeepIncoming, 'd'), + ] { + let preview = PlatformReviewActionPreview::resolve_conflict( + target.clone(), + &review, + &both, + "proposal-conflict", + "file-conflict", + resolution, + ) + .expect("advertised side"); + let (confirmation, correlation) = digests(seed); + let carried = preview.confirmation().expect("confirmed lane"); + assert_eq!( + carried.confirmation_digest(), + &confirmation, + "each side carries its own digest, so they can never be swapped" + ); + assert_eq!(carried.receipt_correlation_digest(), &correlation); + assert!(matches!( + preview.action(), + ReviewAction::ResolveConflict { resolution: sent, .. } if *sent == resolution + )); + assert!(preview.matches_capabilities(&both)); + } + + // A delete/modify conflict has one side recorded, so only that side is + // ever offered and the other refuses locally. + let one_side = capabilities( + &target, + &review, + ReviewGitStagingCapabilities { + staging: Vec::new(), + conflict_resolutions: vec![conflict_capability( + "proposal-conflict", + "file-conflict", + ConflictResolution::KeepCurrent, + 'a', + 'c', + )], + }, + ); + let projection = + ReviewWorktreeProjection::new(&review, Some(&target), Some(&one_side), true); + assert_eq!( + projection.advertised_sides("proposal-conflict", "file-conflict"), + [ConflictResolution::KeepCurrent] + ); + assert!(PlatformReviewActionPreview::resolve_conflict( + target.clone(), + &review, + &one_side, + "proposal-conflict", + "file-conflict", + ConflictResolution::KeepIncoming, + ) + .is_err()); + + // A file the snapshot no longer reports as unmerged cannot be + // collapsed, whatever an older capability read said. + let mut resolved = review.clone(); + resolved + .files + .iter_mut() + .find(|file| file.id == "file-conflict") + .expect("conflicted file") + .conflict = ConflictState::Resolved; assert!( - !conflicted[0].staging[0].admissible, - "an unresolved conflict blocks its own staging proposal" + ReviewWorktreeProjection::new(&resolved, Some(&target), Some(&both), true) + .advertised_conflict_resolutions() + .is_empty() ); } @@ -1111,7 +1840,7 @@ mod tests { fn sdtest_1849_canonical_snapshot_projects_to_lanes_previews_and_observations() { let snapshot = decode_review_snapshot(CANONICAL_FIXTURE).expect("canonical fixture"); let review = PlatformReviewSemantic::from(&snapshot); - let projection = ReviewWorktreeProjection::new(&review, None, true); + let projection = ReviewWorktreeProjection::new(&review, None, None, true); assert_eq!(projection.source_revision, review.revision); assert_eq!(projection.distinct_file_count(), 1); diff --git a/crates/shelldeck-ui/src/fleet_view.rs b/crates/shelldeck-ui/src/fleet_view.rs index 84a5c563..05a49aeb 100644 --- a/crates/shelldeck-ui/src/fleet_view.rs +++ b/crates/shelldeck-ui/src/fleet_view.rs @@ -171,9 +171,12 @@ const fn review_rerun_control_gate( /// /// An empty lane is omitted rather than rendered as a zero-count header: a /// clean index should not look like four unfinished sections. -fn visible_review_lanes(projection: &ReviewWorktreeProjection) -> Vec<&ReviewWorktreeLaneGroup> { - projection - .lanes +/// +/// Takes the lane slice rather than the projection: this decision is about +/// lane contents alone, and the projection also carries the advertisement, +/// which has no bearing on which lanes are drawn. +fn visible_review_lanes(lanes: &[ReviewWorktreeLaneGroup]) -> Vec<&ReviewWorktreeLaneGroup> { + lanes .iter() .filter(|group| !group.files.is_empty()) .collect() @@ -222,10 +225,41 @@ const fn review_staging_withheld_key(reason: ReviewStagingWithheld) -> &'static ReviewStagingWithheld::NoServerCapability => { "fleet.review.staging_withheld_no_server_capability" } + ReviewStagingWithheld::NoStageableProposal => { + "fleet.review.staging_withheld_no_stageable_proposal" + } ReviewStagingWithheld::NoCustodyLane => "fleet.review.staging_withheld_no_custody_lane", } } +/// The label for one side of a conflict, named by what it keeps. +/// +/// A file with both sides recorded renders both, so the two labels have to be +/// distinguishable at a glance rather than differing only by a suffix. +const fn review_conflict_side_key(resolution: ConflictResolution) -> &'static str { + match resolution { + ConflictResolution::KeepCurrent => "fleet.review.resolve_keep_current", + ConflictResolution::KeepIncoming => "fleet.review.resolve_keep_incoming", + } +} + +/// The label for one advertised staging control. +/// +/// Separate from [`localized_review_proposal_kind`], which yields the phrase +/// embedded in a confirmation sentence. A button needs the bare verb, and the +/// three verbs are separately granted, so each gets its own key rather than +/// one parameterised string that could render a grant nobody installed. +const fn review_staging_button_key(kind: ReviewProposalKind) -> &'static str { + match kind { + ReviewProposalKind::Stage => "fleet.review.prepare_stage", + ReviewProposalKind::Unstage => "fleet.review.prepare_unstage", + ReviewProposalKind::Commit => "fleet.review.prepare_commit", + // Unreachable: a conflict resolution is never advertised as a staging + // capability, so it never reaches a staging button. + ReviewProposalKind::ResolveConflict => "fleet.review.action_resolve_conflict", + } +} + const fn review_delivery_withheld_key(reason: ReviewDeliveryWithheld) -> &'static str { match reason { ReviewDeliveryWithheld::NoServerCapability => { @@ -910,6 +944,57 @@ impl FleetView { self.admit_review_preview(preview) } + /// Prepare one confirmed index-level staging transition. + /// + /// The preview takes the kind and both digests from the capability, so + /// this refuses locally whenever the advertisement no longer covers the + /// proposal — including when it covers it at a `HEAD` or an index that has + /// since moved, because the server minted the digest over both. + fn prepare_review_staging(&mut self, proposal_id: &str) -> Result<(), ReviewPrepareRefusal> { + let target = self + .review_target + .clone() + .ok_or(ReviewPrepareRefusal::Preview)?; + let preview = PlatformReviewActionPreview::stage_proposal( + target, + self.available_review() + .map_err(|_| ReviewPrepareRefusal::Preview)?, + self.available_review_capabilities() + .map_err(|_| ReviewPrepareRefusal::Preview)?, + proposal_id, + ) + .map_err(|_| ReviewPrepareRefusal::Preview)?; + self.admit_review_preview(preview) + } + + /// Prepare one confirmed collapse of a conflicted file to a recorded side. + /// + /// `resolution` names which of the two blobs git is already holding would + /// land. No content is supplied here or anywhere below it. + fn prepare_review_conflict_resolution( + &mut self, + proposal_id: &str, + file_id: &str, + resolution: ConflictResolution, + ) -> Result<(), ReviewPrepareRefusal> { + let target = self + .review_target + .clone() + .ok_or(ReviewPrepareRefusal::Preview)?; + let preview = PlatformReviewActionPreview::resolve_conflict( + target, + self.available_review() + .map_err(|_| ReviewPrepareRefusal::Preview)?, + self.available_review_capabilities() + .map_err(|_| ReviewPrepareRefusal::Preview)?, + proposal_id, + file_id, + resolution, + ) + .map_err(|_| ReviewPrepareRefusal::Preview)?; + self.admit_review_preview(preview) + } + fn prepare_review_approval(&mut self) -> Result<(), ReviewPrepareRefusal> { let target = self .review_target @@ -1716,20 +1801,26 @@ impl FleetView { ); let can_note = can_anchor && self.review_notes.is_some(); let selected_anchor = self.selected_review_anchor.as_ref(); + // The lanes render either way; without an exactly reconciled target no + // capability response can be attributed to this read, so every control + // is withheld for want of a server capability. let projection = ReviewWorktreeProjection::new( review, + self.review_target.as_ref(), self.available_review_capabilities().ok(), self.review_custody.is_some(), ); let mut files = div().flex().flex_col().gap(px(10.0)); - for group in visible_review_lanes(&projection) { + for group in visible_review_lanes(&projection.lanes) { let mut lane_files = div().flex().flex_col().gap(px(6.0)); for file in &group.files { lane_files = lane_files.child(Self::render_review_file( group.lane, file, + &projection, selected_anchor, can_anchor, + can_prepare, &entity, )); } @@ -1755,13 +1846,45 @@ impl FleetView { .child(lane_files), ); } + // The lane-level answer, shown only when the snapshot holds something + // a staging control could have applied to. A review with no actionable + // proposal and no unresolved conflict has no gap to explain, and a + // permanent "nothing to stage" banner would be noise rather than an + // answer — the same rule the delivery sibling follows. if let Err(reason) = projection.staging { + let has_local_candidate = review.proposals.iter().any(|proposal| { + review.proposal_is_actionable(&proposal.id) + || proposal.files.iter().any(|file_id| { + review.conflict_resolution_is_actionable(&proposal.id, file_id) + }) + }); + if has_local_candidate { + files = files.child( + div().min_w(px(0.0)).overflow_hidden().child( + Alert::info() + .title(t!("fleet.review.staging_withheld_title").to_string()) + .description(t!(review_staging_withheld_key(reason)).to_string()), + ), + ); + } + } else if let Some(observation) = projection.observation() { + // What the server read, so a reader can see the fence rather than + // trust that one exists. The controls below are bound to exactly + // this `HEAD` and this index. files = files.child( - div().min_w(px(0.0)).overflow_hidden().child( - Alert::info() - .title(t!("fleet.review.staging_withheld_title").to_string()) - .description(t!(review_staging_withheld_key(reason)).to_string()), - ), + div() + .min_w(px(0.0)) + .overflow_hidden() + .text_size(px(10.0)) + .text_color(ShellDeckColors::text_muted()) + .child( + t!( + "fleet.review.staging_observation", + head = observation.head_revision.as_str(), + index = observation.index_digest.as_str() + ) + .to_string(), + ), ); } @@ -2402,8 +2525,10 @@ impl FleetView { fn render_review_file( lane: ReviewWorktreeLane, file: &ReviewWorktreeFile, + projection: &ReviewWorktreeProjection, selected_anchor: Option<&ReviewAnchorSemantic>, can_anchor: bool, + can_prepare: bool, entity: &Entity, ) -> AnyElement { let mut hunks = div().flex().flex_col().gap(px(4.0)); @@ -2490,8 +2615,20 @@ impl FleetView { .variant(BadgeVariant::Warning), ); } - // Server-proposed staging transitions are shown as observations. No - // control accompanies them: see `projection.staging`. + // Server-proposed staging transitions, shown as observations. A + // control joins one only where the server advertised a slot for that + // exact proposal; the rest stay observations. That is the whole of the + // withholding rule: a deployment granting index writes but not + // committing advertises `Stage` and `Unstage` and no `Commit`, so the + // commit proposal keeps its badge and simply grows no button. + // + // These sit on the file card rather than in the footer's + // `effect_controls` row, unlike the check rerun and the batch send: a + // staging proposal names files, so the control belongs beside the + // badge naming it. Same widgets and same sizing as those two — the + // placement is the only divergence. + let mut staging_controls = div().flex().items_center().flex_wrap().gap(px(5.0)); + let mut has_staging_control = false; for proposal in &file.staging { let action = localized_review_proposal_kind(proposal.kind); header = header.child( @@ -2517,6 +2654,82 @@ impl FleetView { BadgeVariant::Destructive }), ); + if projection.advertises_staging(&proposal.proposal_id) { + has_staging_control = true; + let stage_entity = entity.clone(); + let stage_id = proposal.proposal_id.clone(); + staging_controls = staging_controls.child( + Button::new( + ElementId::from(SharedString::from(format!( + "review-stage-{}-{}-{}", + lane.as_str(), + file.id, + proposal.proposal_id + ))), + t!(review_staging_button_key(proposal.kind)).to_string(), + ) + .size(ButtonSize::Sm) + .variant(ButtonVariant::Outline) + .disabled(!can_prepare) + .on_click(move |_, _, cx| { + stage_entity.update(cx, |this, cx| { + if let Err(refusal) = this.prepare_review_staging(&stage_id) { + this.review_receipt = Some(( + t!("fleet.review.state_refused").to_string(), + refusal.detail(), + )); + } + cx.notify(); + }); + }), + ); + } + // One control per admissible side, because the server advertises + // one entry per side git actually recorded and the side is inside + // the confirmation digest. A file holding both stage 2 and stage 3 + // therefore renders two buttons; a delete/modify pair renders one. + for side in projection.advertised_sides(&proposal.proposal_id, &file.id) { + has_staging_control = true; + let resolve_entity = entity.clone(); + let resolve_proposal = proposal.proposal_id.clone(); + let resolve_file = file.id.clone(); + staging_controls = staging_controls.child( + Button::new( + ElementId::from(SharedString::from(format!( + "review-resolve-{}-{}-{}-{}", + lane.as_str(), + file.id, + proposal.proposal_id, + side.as_str() + ))), + t!(review_conflict_side_key(side)).to_string(), + ) + .size(ButtonSize::Sm) + .variant(ButtonVariant::Outline) + .disabled(!can_prepare) + .on_click(move |_, _, cx| { + resolve_entity.update(cx, |this, cx| { + if let Err(refusal) = this.prepare_review_conflict_resolution( + &resolve_proposal, + &resolve_file, + side, + ) { + this.review_receipt = Some(( + t!("fleet.review.state_refused").to_string(), + refusal.detail(), + )); + } + cx.notify(); + }); + }), + ); + } + } + + // Only claim the row's height when it holds something: an empty + // control row would still push the preview down by its own margin. + if has_staging_control { + staging_controls = staging_controls.mb(px(6.0)); } div() @@ -2527,6 +2740,7 @@ impl FleetView { .border_color(ShellDeckColors::border()) .rounded(px(6.0)) .child(header) + .child(staging_controls) .child(Self::render_review_preview(&file.preview)) .child(hunks) .into_any_element() @@ -3971,10 +4185,11 @@ impl Render for FleetView { mod review_render_tests { use super::{ exact_review_target_index, fleet_uses_compact_layout, review_anchor_element_id, - 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, + review_conflict_side_key, 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_button_key, + review_staging_withheld_key, same_exact_review_snapshot, semantic_words, + visible_review_lanes, BadgeVariant, ConflictResolution, ReviewDeliveryWithheld, ReviewDispatchDirective, ReviewPrepareRefusal, }; use shelldeck_core::config::platform_review::PlatformReviewTarget; @@ -4151,8 +4366,8 @@ mod review_render_tests { fn sdtest_1850_only_populated_lanes_render_and_conflicts_lead() { use shelldeck_core::config::platform_review::{ review_safe_text, ConflictState, DiffChangeKind, ReviewPreviewWithheld, - ReviewSafePreview, ReviewStagingWithheld, ReviewWorktreeFile, ReviewWorktreeLane, - ReviewWorktreeLaneGroup, ReviewWorktreeProjection, WorktreeFileState, + ReviewSafePreview, ReviewWorktreeFile, ReviewWorktreeLane, ReviewWorktreeLaneGroup, + WorktreeFileState, }; let file = |id: &str| ReviewWorktreeFile { @@ -4166,35 +4381,31 @@ mod review_render_tests { hunks: Vec::new(), staging: Vec::new(), }; - let projection = ReviewWorktreeProjection { - source_revision: automonique_protocol::primitives::Revision::new(9).unwrap(), - lanes: vec![ - ReviewWorktreeLaneGroup { - lane: ReviewWorktreeLane::Conflicted, - semantic_key: ReviewWorktreeLane::Conflicted.semantic_key(), - files: vec![file("file-conflict")], - }, - ReviewWorktreeLaneGroup { - lane: ReviewWorktreeLane::Staged, - semantic_key: ReviewWorktreeLane::Staged.semantic_key(), - files: Vec::new(), - }, - ReviewWorktreeLaneGroup { - lane: ReviewWorktreeLane::Unstaged, - semantic_key: ReviewWorktreeLane::Unstaged.semantic_key(), - files: vec![file("file-unstaged")], - }, - ReviewWorktreeLaneGroup { - lane: ReviewWorktreeLane::Untracked, - semantic_key: ReviewWorktreeLane::Untracked.semantic_key(), - files: Vec::new(), - }, - ], - staging: Err(ReviewStagingWithheld::NoServerCapability), - }; + let lanes = vec![ + ReviewWorktreeLaneGroup { + lane: ReviewWorktreeLane::Conflicted, + semantic_key: ReviewWorktreeLane::Conflicted.semantic_key(), + files: vec![file("file-conflict")], + }, + ReviewWorktreeLaneGroup { + lane: ReviewWorktreeLane::Staged, + semantic_key: ReviewWorktreeLane::Staged.semantic_key(), + files: Vec::new(), + }, + ReviewWorktreeLaneGroup { + lane: ReviewWorktreeLane::Unstaged, + semantic_key: ReviewWorktreeLane::Unstaged.semantic_key(), + files: vec![file("file-unstaged")], + }, + ReviewWorktreeLaneGroup { + lane: ReviewWorktreeLane::Untracked, + semantic_key: ReviewWorktreeLane::Untracked.semantic_key(), + files: Vec::new(), + }, + ]; assert_eq!( - visible_review_lanes(&projection) + visible_review_lanes(&lanes) .iter() .map(|group| group.lane) .collect::>(), @@ -4369,4 +4580,95 @@ mod review_render_tests { ); } } + + // SDTEST-1872 — SDUC-495. Every staging control the surface can paint has + // its own label in both shipped locales, and the three staging verbs never + // collapse onto one string. + // + // They are separately granted: a deployment can install `index_write` + // without `commit`, so a shared label would let a reader believe the + // control they are looking at is the one they were denied. The two + // conflict sides matter for the same reason — a file with both recorded + // renders both buttons side by side, and only the label distinguishes + // which blob would land. + #[test] + fn sdtest_1872_every_staging_control_has_its_own_label_in_both_shipped_locales() { + use shelldeck_core::config::platform_review::ReviewProposalKind; + use std::collections::BTreeSet; + + let keys = [ + review_staging_button_key(ReviewProposalKind::Stage), + review_staging_button_key(ReviewProposalKind::Unstage), + review_staging_button_key(ReviewProposalKind::Commit), + review_conflict_side_key(ConflictResolution::KeepCurrent), + review_conflict_side_key(ConflictResolution::KeepIncoming), + ]; + assert_eq!( + keys.iter().collect::>().len(), + keys.len(), + "two separately granted controls must not share one label" + ); + + 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}"); + assert!(!copy.trim().is_empty(), "{locale} {key} is empty"); + copy + }) + .collect::>(); + assert_eq!(copies.len(), keys.len(), "{locale} reuses one label"); + } + } + + // SDTEST-1873 — SDUC-495. The staging lane's own withheld reasons stay + // distinct from each other and from the delivery lane's, including the new + // "the server proved nothing" answer that only exists once a capability + // response can be attributed at all. + #[test] + fn sdtest_1873_staging_withheld_reasons_stay_distinct_from_the_delivery_lane() { + use shelldeck_core::config::platform_review::ReviewStagingWithheld; + use std::collections::BTreeSet; + + let staging = ReviewStagingWithheld::ALL + .iter() + .map(|reason| review_staging_withheld_key(*reason)) + .collect::>(); + assert_eq!(staging.len(), ReviewStagingWithheld::ALL.len()); + + for locale in ["fr", "en"] { + let staging_copies = staging + .iter() + .map(|key| { + let copy = crate::t!(*key, locale = locale).to_string(); + assert_ne!(©, *key, "{locale} is missing {key}"); + copy + }) + .collect::>(); + assert_eq!( + staging_copies.len(), + staging.len(), + "{locale} reuses one staging explanation" + ); + for reason in ReviewDeliveryWithheld::ALL { + let delivery = + crate::t!(review_delivery_withheld_key(reason), locale = locale).to_string(); + assert!( + !staging_copies.contains(&delivery), + "{locale} explains staging with the delivery lane's copy" + ); + } + // The retired copy named a contract gap that no longer exists: the + // server now defines both staging digests. Shipping it would tell + // the user to wait for something that already landed. + for copy in &staging_copies { + assert!( + !copy.contains("Platform v2") && !copy.contains("hunk"), + "{locale} still explains staging by the closed contract gap: {copy}" + ); + } + } + } } diff --git a/docs/testing/USE_CASES.md b/docs/testing/USE_CASES.md index e93305e5..165beca1 100644 --- a/docs/testing/USE_CASES.md +++ b/docs/testing/USE_CASES.md @@ -2936,12 +2936,41 @@ 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. 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. +receipt-correlation digest. Every exposed confirmation names the exact +workspace and captured snapshot or target revision before dispatch. + +File-level staging, unstaging, committing and conflict resolution are exposed +on the same terms, against `ReviewCapabilities`' `staging` and +`conflict_resolutions` lists. Each entry names the commit `HEAD` the server's +preflight resolved to and a digest over the whole index it read, because a +worktree is shared substrate: another process running as the daemon uid can +move either between the advertisement and the action, and the snapshot revision +tracks only what the projection observed. The four actions require a +server-minted confirmation, so their unconfirmed spelling cannot be encoded on +either side of the wire, and their receipts are recovered through the +correlation digest rather than by re-sending an action whose confirmation the +write itself invalidated. A capability whose observation disagrees with a +fresher document stops matching, so the control withdraws locally rather than +being refused by the daemon. + +A control exists only where a slot exists for that exact proposal. The three +staging kinds share one capability type but not one grant — an operator +installs index writes, committing and conflict resolution independently — so a +deployment that grants stage and unstage but not commit produces two controls +and not three, and the withheld one is *absent* rather than disabled. Absence +of the whole lane is reported with its own stated reason: no capability +response attributed to this coordinate, an exact response that advertised +nothing, or no durable custody lane. `conflict_resolutions` lists one entry per +side git actually recorded, because the side decides which blob lands and is +inside the confirmation digest, so a file holding both sides renders two +controls and a delete/modify pair renders one; nothing but a side ever crosses +that wire, so no caller-supplied content can be written. + +Hunk-level staging remains unrepresentable: a proposal names file ids, and +`ReviewAnchor`'s hunk id serves comments only. The three pull-request slots +still ship empty because no provider adapter can preflight one, and the merge +preview is now a confirmed lane too, so it refuses at construction rather than +minting a request the transport cannot encode. The read side presents those files as one combined worktree: conflicted first, then staged, unstaged, and untracked, with empty lanes omitted. A partially @@ -2950,11 +2979,9 @@ only as conflicted, because git reports it unmerged and refuses to stage it, while a resolved conflict returns to its own lane. Anchor identities bind lane, 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` defines no staging capability, confirmation digest, or -receipt-correlation digest, and the action preview structurally refuses a -confirmation on those four actions. Hunk-level +proposals are shown as per-file observations with their admissibility; a +control joins one only where the server advertised a slot for that exact +proposal, and the surface otherwise states which fence is missing. 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 @@ -2990,6 +3017,51 @@ an unavailable adapter refuses before any effect. ## Change log +- **2026-08-29** — Amended SDUC-495 with capability-fenced file-level staging, + unstaging, committing and conflict resolution, and added SDTEST-1867..1873. + This was the last open item of `benfavre/shelldeck#128`, and it was blocked + on the same kind of contract gap as the batch delivery before it: a proposal + id plus a snapshot revision does not pin a worktree, so a staging + confirmation digest would have had to commit to the `HEAD` object id and the + index state, and no projection carried either. `bext-stack/automonique` + PR #225 earned that field set from a mutation-free preflight of the + repository, and ShellDeck now consumes it. + + The controls follow `delivery.rs` rather than a second shape: a read-side + projection reporting what the server proved, plus the exact missing fence + when it proved nothing. Absence *is* the withholding — the three staging + kinds share one capability type but not one grant, so a deployment that + installs index writes without committing advertises no `Commit` entry and + that proposal keeps its observation badge and grows no button. SDTEST-1867 + demonstrates that rather than asserting it: the same run drives stage and + unstage end to end to a dispatchable confirmed preview, then grants the + commit and watches the third control appear from unchanged projection code. + + `conflict_resolutions` lists one entry per side git actually recorded, + because the side decides which blob lands and is therefore inside the + confirmation digest; a file holding both sides renders two controls + (SDTEST-1869). A stale worktree withdraws the control locally: the server + mints the digest over the commit `HEAD`, the whole index and every named + path, so comparing it against a fresher capability document is the + head-and-index comparison, and the observation is exposed so a reader can see + the fence rather than trust one exists (SDTEST-1868). + + Two findings worth recording. The confirmation guard in `review_action` was a + local `matches!` on the rerun rather than the contract's own + `requires_confirmation()`; the confirmed set grew from one member to eight + with this protocol revision, and the stale spelling was already letting + `merge_pull_request` mint previews the transport could no longer encode. And + the custody disk format did not understand any of the four staging families, + so preparation would have returned "unsupported action" and the dispatch + would have died at the fence — the same defect PR #161 found for the batch + delivery after PR #160 asserted the format already knew "these actions". + SDTEST-1870/1871 drive the whole store rather than only the enum. + + Hunk-level staging is still not representable and was not invented: proposals + list file ids, and `ReviewAnchor`'s hunk id serves comments only. The #128 + item therefore ticks for the file half and needs the hunk half called out as + a protocol follow-up. + - **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` diff --git a/docs/testing/tests-core.md b/docs/testing/tests-core.md index f03c50ad..fb7c0b28 100644 --- a/docs/testing/tests-core.md +++ b/docs/testing/tests-core.md @@ -552,7 +552,7 @@ Existing: **0 tests**. | SDTEST-1845 | `config/platform_review/worktree.rs::sdtest_1845_safe_text_neutralizes_invisible_reordering_and_bounds_the_shape` | SDUC-495 | Green | The protocol bound (512 control-free bytes) still admits bidirectional overrides and zero-width scalars that repaint a diff line. The last stop before pixels replaces them with U+FFFD, splits on both line terminators, and caps line count and line length with an explicit truncation marker. | | SDTEST-1846 | `config/platform_review/worktree.rs::sdtest_1846_image_previews_are_clamped_and_absurd_rasters_are_refused` | SDUC-495 | Green | Declared raster dimensions are a claim, not a budget. A sanitized bounded image yields an aspect-preserving placeholder box never above the edge cap (including an extreme 4000×1 ratio); zero, over-edge, over-area and incomplete dimensions are withheld as oversized raster, and a missing or over-budget byte size is withheld as oversized. | | SDTEST-1847 | `config/platform_review/worktree.rs::sdtest_1847_html_and_opaque_previews_are_described_but_never_rendered` | SDUC-495 | Green | Sanitized HTML yields a description carrying no markup and no source text. Unsanitized payloads (HTML or text), a media type disagreeing with the kind, hunks on a non-text kind, opaque binary, an absent preview, and a text preview with no hunk content are each withheld with their own reason. | -| SDTEST-1848 | `config/platform_review/worktree.rs::sdtest_1848_staging_stays_withheld_for_every_capability_load` | SDUC-495 | Green | A snapshot proposal carrying a `git` authority is an observation. `advertised_staging_capability` is false for every capability load, so the projection reports `NoServerCapability` before the custody fence and exposes staging only as a per-file proposal observation; an unresolved conflict marks its own proposal inadmissible. | +| SDTEST-1848 | *(retired 2026-08-29)* | SDUC-495 | Retired | Pinned the reason staging had no control: `advertised_staging_capability` returned false for every capability load because Platform v2 defined no staging capability. `bext-stack/automonique` PR #225 closed that gap, so the pin no longer describes anything. SDTEST-1867 replaces it and fails closed the other way — it proves a *withheld* grant renders no control while its granted siblings go end to end. | | SDTEST-1849 | `config/platform_review/worktree.rs::sdtest_1849_canonical_snapshot_projects_to_lanes_previews_and_observations` | SDUC-495 | Green | The shipped canonical v2 fixture — not a hand-built snapshot — reaches the combined lanes (its partially staged file on both sides, no conflicts, no untracked), the bounded safe text preview, the new-side anchor, the commit proposal observation, and the withheld staging fence. | | SDTEST-1853 | `config/platform_review/notes.rs::note_and_selection_survive_restart_with_exact_anchor` | SDUC-495 | Green | A line-anchored note and its batch-delivery selection survive restart with the exact file/hunk/side/line coordinates and the snapshot revision they were captured against. | | 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. | @@ -561,6 +561,11 @@ Existing: **0 tests**. | 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. | +| SDTEST-1867 | `config/platform_review/worktree.rs::sdtest_1867_withheld_commit_grant_renders_no_control_while_its_siblings_go_end_to_end` | SDUC-495 | Green | The withholding demonstration, not an assertion about it. A deployment that installs index writes but not committing advertises `Stage` and `Unstage` and no `Commit`: the commit proposal keeps its observation badge, `advertises_staging` answers false, and the preview constructor refuses it, while both granted kinds go all the way to a confirmed preview carrying the server's own confirmation and correlation digests verbatim and still admitted by a re-read of the capability. Granting the commit then makes the third control appear from unchanged projection code. Mutation-checked: forcing `advertises_staging` true fails the test. | +| SDTEST-1868 | `config/platform_review/worktree.rs::sdtest_1868_staging_lane_names_its_missing_fence_and_refuses_a_moved_worktree` | SDUC-495 | Green | Each staging fence fails closed with its own reason: no capability response, no exactly reconciled target, an exact response with two empty git lists, and a missing custody lane are four distinct answers. The lanes still render in every one of them — only the control is withheld. The observed `HEAD` and index digest are exposed so a reader can see the fence, and a preview minted against one worktree read stops matching once a fresher document reports a different `HEAD` *or* a different index, so the control withdraws locally instead of being refused by the daemon. A capability whose kind disagrees with the snapshot's proposal is dropped as a torn read. | +| SDTEST-1869 | `config/platform_review/worktree.rs::sdtest_1869_both_recorded_sides_render_two_controls_and_a_single_side_renders_one` | SDUC-495 | Green | `conflict_resolutions` lists one entry per admissible side, so a file with both stage 2 and stage 3 recorded yields two controls and each carries the digest minted over the blob that side would write — proved by asserting the two previews carry *different* confirmations, which is what makes them unswappable. A delete/modify pair advertises one side and the other refuses locally. A file the snapshot no longer reports unmerged advertises nothing, whatever an older capability read said. | +| SDTEST-1870 | `config/platform_review/custody.rs::confirmed_staging_is_fenced_and_survives_the_disk_round_trip` | SDUC-495 | Green | A confirmed staging preview takes the same durable at-most-once lane as every other exposed mutation: a second staging cannot open a parallel effect, a restart before dispatch reports it never started and after dispatch is lookup-only, and both server digests survive the disk round trip. The disk format did not know these four families before this slice, so `prepare` would have returned `UnsupportedAction` and the dispatch would have died at the fence — the same defect PR #161 found for the batch delivery, which is why this drives the whole store rather than only the enum. | +| SDTEST-1871 | `config/platform_review/custody.rs::every_staging_family_round_trips_and_a_resolution_keeps_its_side` | SDUC-495 | Green | Each of `Stage`, `Unstage`, `Commit` and both sides of `ResolveConflict` serializes and re-parses as exactly itself through the protocol's own grammar. The three staging kinds are separate disk variants because they are separately granted, so a recovered plan cannot be re-read as a wider one, and a resolution persists its side because the side decides which blob lands. A record whose confirmation no longer matches its family is rejected as invalid rather than recovered into a preview the transport cannot encode. | --- diff --git a/docs/testing/tests-ui-and-app.md b/docs/testing/tests-ui-and-app.md index ddd16f34..d9500b39 100644 --- a/docs/testing/tests-ui-and-app.md +++ b/docs/testing/tests-ui-and-app.md @@ -545,6 +545,8 @@ parallel `cargo test`. | 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-1872 | `fleet_view.rs::review_render_tests::sdtest_1872_every_staging_control_has_its_own_label_in_both_shipped_locales` | SDUC-495 | Green | Stage, unstage, commit and the two conflict sides each resolve to their own non-empty localized label in both `fr.toml` and `en.toml`, and none reuses another's copy. They are separately granted, so a shared label would let a reader believe the control in front of them is the one their deployment denied; and a file with both conflict sides recorded renders both buttons side by side, where only the label says which blob would land. | +| SDTEST-1873 | `fleet_view.rs::review_render_tests::sdtest_1873_staging_withheld_reasons_stay_distinct_from_the_delivery_lane` | SDUC-495 | Green | The three staging withheld reasons resolve to distinct localized copy in both locales, never reuse a delivery-lane explanation, and no longer name the contract gap that PR #225 closed — a surface still telling the user to wait for a capability that already shipped would send them to a fix that does not exist. | | 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. |