From 4d0f2afc1e961c7186b8b0ec9b2ff0de6e563fe2 Mon Sep 17 00:00:00 2001 From: Hare Date: Thu, 8 Oct 2026 15:44:26 +0900 Subject: [PATCH] fix(merge-request): share immutable integration and requirement evidence T-716: validate recorded source/approval/request identity, chronology and revocation independently of mutable selectors. Reuse exact revision and complete linked-source attestation in CompleteTicket. Assignment and lifecycle mutation are unchanged.\n\nValidation: cargo test -p merge-request (9 evidence and 21 store tests). --- crates/merge-request/src/lib.rs | 319 +++++++++++++---- crates/merge-request/tests/evidence.rs | 459 +++++++++++++++++++++++++ crates/merge-request/tests/store.rs | 140 ++++++++ 3 files changed, 850 insertions(+), 68 deletions(-) create mode 100644 crates/merge-request/tests/evidence.rs diff --git a/crates/merge-request/src/lib.rs b/crates/merge-request/src/lib.rs index 78a40bbd5..b23465576 100644 --- a/crates/merge-request/src/lib.rs +++ b/crates/merge-request/src/lib.rs @@ -236,10 +236,244 @@ pub struct MergeRequest { #[serde(default)] pub thread: Vec, } +/// Bounded failures from pure evaluation of persisted review and merge evidence. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum MergeRequestEvidenceError { + NotMerged, + MergeResultMissing, + ApprovalMissing, + ApprovalRevoked, + ApprovalNotApproved, + ApprovalSourceMismatch, + ApprovalTimelineMismatch, + ReviewRequestMissing, + ReviewRequestMismatch, + ApprovalNotEffective, + ItemRevisionMismatch, + SourceSnapshotMismatch, + RequirementApprovalMissing, +} + +impl MergeRequestEvidenceError { + /// Stable machine-readable diagnostic code, separate from the human message. + pub fn code(self) -> &'static str { + match self { + Self::NotMerged => "not_merged", + Self::MergeResultMissing => "merge_result_missing", + Self::ApprovalMissing => "approval_missing", + Self::ApprovalRevoked => "approval_revoked", + Self::ApprovalNotApproved => "approval_not_approved", + Self::ApprovalSourceMismatch => "approval_source_mismatch", + Self::ApprovalTimelineMismatch => "approval_timeline_mismatch", + Self::ReviewRequestMissing => "review_request_missing", + Self::ReviewRequestMismatch => "review_request_mismatch", + Self::ApprovalNotEffective => "approval_not_effective", + Self::ItemRevisionMismatch => "item_revision_mismatch", + Self::SourceSnapshotMismatch => "source_snapshot_mismatch", + Self::RequirementApprovalMissing => "requirement_approval_missing", + } + } + + pub fn as_str(self) -> &'static str { + match self { + Self::NotMerged => "Merge Request has no merged result", + Self::MergeResultMissing => "merged Merge Request has no MergeResult", + Self::ApprovalMissing => "integration approval is missing", + Self::ApprovalRevoked => "Reviewer approval is revoked", + Self::ApprovalNotApproved => "Reviewer decision is not an approval", + Self::ApprovalSourceMismatch => "Reviewer approval does not match the exact source ref", + Self::ApprovalTimelineMismatch => { + "integration approval does not predate the merged result" + } + Self::ReviewRequestMissing => "linked ReviewRequested evidence is missing", + Self::ReviewRequestMismatch => { + "Reviewer approval does not match linked ReviewRequested evidence" + } + Self::ApprovalNotEffective => "Reviewer approval is not the effective review", + Self::ItemRevisionMismatch => { + "Reviewer approval does not attest the current Ticket revision" + } + Self::SourceSnapshotMismatch => { + "Reviewer approval does not attest the exact linked source set" + } + Self::RequirementApprovalMissing => { + "no effective Reviewer approval attests the current Ticket revision and exact linked source set" + } + } + } +} + +impl std::fmt::Display for MergeRequestEvidenceError { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter.write_str(self.as_str()) + } +} + +impl std::error::Error for MergeRequestEvidenceError {} + impl MergeRequest { pub fn effective_review(&self, subject: &str) -> Option<&ReviewEvent> { self.thread.iter().rev().find_map(|e|match e{MergeRequestThreadEvent::Review(v)if v.subject_ref==subject&&!self.thread.iter().any(|x|matches!(x,MergeRequestThreadEvent::ReviewRevoked(r)if r.review_event_id==v.event_id))=>Some(v),_=>None}) } + + /// Returns the recorded merged result, without resolving selectors or mutating state. + pub fn merged_result(&self) -> Result<&MergeEvent, MergeRequestEvidenceError> { + if self.state != MergeRequestState::Merged { + return Err(MergeRequestEvidenceError::NotMerged); + } + self.thread + .iter() + .rev() + .find_map(|event| match event { + MergeRequestThreadEvent::Merge(event) => Some(event), + _ => None, + }) + .ok_or(MergeRequestEvidenceError::MergeResultMissing) + } + + /// Validates the specific approval recorded by the merged result. A later + /// requirement review does not replace this integration evidence. + pub fn integration_approval(&self) -> Result<&ReviewEvent, MergeRequestEvidenceError> { + let merge = self.merged_result()?; + let review = self + .thread + .iter() + .find_map(|event| match event { + MergeRequestThreadEvent::Review(review) + if review.event_id == merge.approval_event_id => + { + Some(review) + } + _ => None, + }) + .ok_or(MergeRequestEvidenceError::ApprovalMissing)?; + if merge.approved_source_ref.trim().is_empty() + || review.subject_ref != merge.approved_source_ref + { + return Err(MergeRequestEvidenceError::ApprovalSourceMismatch); + } + if review.sequence >= merge.sequence { + return Err(MergeRequestEvidenceError::ApprovalTimelineMismatch); + } + self.approval_evidence(review)?; + Ok(review) + } + + fn approval_evidence(&self, review: &ReviewEvent) -> Result<(), MergeRequestEvidenceError> { + if self.thread.iter().any(|event| { + matches!(event, MergeRequestThreadEvent::ReviewRevoked(revoked) + if revoked.review_event_id == review.event_id) + }) { + return Err(MergeRequestEvidenceError::ApprovalRevoked); + } + if review.decision != ReviewDecision::Approve { + return Err(MergeRequestEvidenceError::ApprovalNotApproved); + } + let requested = self + .thread + .iter() + .find_map(|event| match event { + MergeRequestThreadEvent::ReviewRequested(requested) + if requested.event_id == review.request_event_id => + { + Some(requested) + } + _ => None, + }) + .ok_or(MergeRequestEvidenceError::ReviewRequestMissing)?; + if requested.subject_ref != review.subject_ref + || requested.ticket_item_revision != review.ticket_item_revision + || requested.ticket_merge_request_subjects != review.ticket_merge_request_subjects + || requested.reviewer != review.reviewer + || requested.sequence >= review.sequence + { + return Err(MergeRequestEvidenceError::ReviewRequestMismatch); + } + Ok(()) + } +} + +/// Finds an effective approval of the authoritative current Ticket revision and +/// exact linked source snapshot. Callers supply all linked requests and resolved +/// source refs (recorded merged refs for completion); this function does not read +/// authority, resolve selectors, require merged state, or mutate lifecycle state. +/// An explicit event id validates that approval only; `None` discovers any valid +/// attestation. Snapshot ordering is immaterial, but duplicate ids are rejected. +pub fn requirement_approval<'a>( + requests: &'a [MergeRequest], + item_revision: &str, + subjects: &[MergeRequestReviewSubject], + event_id: Option<&str>, +) -> Result<&'a ReviewEvent, MergeRequestEvidenceError> { + if requests.len() != subjects.len() + || !same_source_snapshot(subjects, subjects) + || requests.iter().any(|request| { + requests + .iter() + .filter(|other| other.merge_request_id == request.merge_request_id) + .count() + != 1 + || !subjects + .iter() + .any(|subject| subject.merge_request_id == request.merge_request_id) + }) + { + return Err(MergeRequestEvidenceError::SourceSnapshotMismatch); + } + for request in requests { + for event in request.thread.iter().rev() { + let MergeRequestThreadEvent::Review(review) = event else { + continue; + }; + if event_id.is_some_and(|id| id != review.event_id) { + continue; + } + let evidence = (|| { + request.approval_evidence(review)?; + if !request + .effective_review(&review.subject_ref) + .is_some_and(|effective| effective.event_id == review.event_id) + { + return Err(MergeRequestEvidenceError::ApprovalNotEffective); + } + if review.ticket_item_revision != item_revision || item_revision.trim().is_empty() { + return Err(MergeRequestEvidenceError::ItemRevisionMismatch); + } + if !same_source_snapshot(&review.ticket_merge_request_subjects, subjects) { + return Err(MergeRequestEvidenceError::SourceSnapshotMismatch); + } + if !subjects.iter().any(|subject| { + subject.merge_request_id == request.merge_request_id + && subject.subject_ref == review.subject_ref + }) { + return Err(MergeRequestEvidenceError::ApprovalSourceMismatch); + } + Ok(review) + })(); + if evidence.is_ok() || event_id.is_some() { + return evidence; + } + } + } + Err(MergeRequestEvidenceError::RequirementApprovalMissing) +} + +fn same_source_snapshot( + left: &[MergeRequestReviewSubject], + right: &[MergeRequestReviewSubject], +) -> bool { + !left.is_empty() + && left.len() == right.len() + && left.iter().all(|subject| { + !subject.merge_request_id.trim().is_empty() + && !subject.subject_ref.trim().is_empty() + && left + .iter() + .filter(|other| other.merge_request_id == subject.merge_request_id) + .count() + == 1 + && right.iter().filter(|other| *other == subject).count() == 1 + }) } #[derive(Clone, Debug, Default)] @@ -1201,81 +1435,30 @@ impl MergeRequestStore { for merge_request_id in &actual_ids { let request = load_mr(&transaction, &input.auth.workspace_id, merge_request_id)? .ok_or(MergeRequestError::NotFound)?; - if request.state != MergeRequestState::Merged { - return Err(MergeRequestError::NotReady(format!( - "Merge Request `{merge_request_id}` has no merged result" - ))); - } + request.integration_approval().map_err(|error| { + let message = format!("Merge Request `{merge_request_id}`: {}", error.as_str()); + if error == MergeRequestEvidenceError::MergeResultMissing { + MergeRequestError::Corrupt(message) + } else { + MergeRequestError::NotReady(message) + } + })?; let merge = request - .thread - .iter() - .rev() - .find_map(|event| match event { - MergeRequestThreadEvent::Merge(event) => Some(event), - _ => None, - }) - .ok_or_else(|| { - MergeRequestError::Corrupt(format!( - "merged Merge Request `{merge_request_id}` has no MergeResult" - )) - })?; - let review = request - .thread - .iter() - .find_map(|event| match event { - MergeRequestThreadEvent::Review(review) - if review.event_id == merge.approval_event_id - && review.subject_ref == merge.approved_source_ref => - { - Some(review) - } - _ => None, - }) - .filter(|review| { - !request.thread.iter().any(|event| { - matches!( - event, - MergeRequestThreadEvent::ReviewRevoked(revoked) - if revoked.review_event_id == review.event_id - ) - }) - }) - .ok_or_else(|| { - MergeRequestError::NotReady(format!( - "Merge Request `{merge_request_id}` integration approval is missing or revoked" - )) - })?; - if review.decision != ReviewDecision::Approve { - return Err(MergeRequestError::NotReady(format!( - "Merge Request `{merge_request_id}` lacks an approved integration result" - ))); - } + .merged_result() + .map_err(|error| MergeRequestError::Corrupt(error.as_str().into()))?; merged_subjects.push(MergeRequestReviewSubject { merge_request_id: merge_request_id.clone(), subject_ref: merge.approved_source_ref.clone(), }); requests.push(request); } - let has_requirement_attestation = requests.iter().any(|request| { - request.thread.iter().rev().any(|event| { - let MergeRequestThreadEvent::Review(review) = event else { - return false; - }; - review.event_id == input.requirement_approval_event_id - && review.decision == ReviewDecision::Approve - && review.ticket_item_revision == input.item_revision - && review.ticket_merge_request_subjects == merged_subjects - && request - .effective_review(&review.subject_ref) - .is_some_and(|effective| effective.event_id == review.event_id) - }) - }); - if !has_requirement_attestation { - return Err(MergeRequestError::NotReady( - "no effective Reviewer approval attests the current Ticket revision and exact merged source set" - .into(), - )); - } + requirement_approval( + &requests, + &input.item_revision, + &merged_subjects, + Some(&input.requirement_approval_event_id), + ) + .map_err(|error| MergeRequestError::NotReady(error.as_str().into()))?; let updated = transaction.execute( "UPDATE typed_tickets SET workflow_state='done',workflow_state_explicit=1,updated_at=?3 diff --git a/crates/merge-request/tests/evidence.rs b/crates/merge-request/tests/evidence.rs new file mode 100644 index 000000000..feaf6337d --- /dev/null +++ b/crates/merge-request/tests/evidence.rs @@ -0,0 +1,459 @@ +use chrono::{TimeZone, Utc}; +use merge_request::*; + +fn actor() -> WorkerIdentity { + WorkerIdentity { + runtime_id: "runtime".into(), + worker_id: "reviewer".into(), + } +} + +fn subjects() -> Vec { + vec![ + MergeRequestReviewSubject { + merge_request_id: "MR-1".into(), + subject_ref: "source-one".into(), + }, + MergeRequestReviewSubject { + merge_request_id: "MR-2".into(), + subject_ref: "source-two".into(), + }, + ] +} + +fn add_review( + request: &mut MergeRequest, + event_id: &str, + revision: &str, + snapshot: &[MergeRequestReviewSubject], + decision: ReviewDecision, +) { + let subject = snapshot + .iter() + .find(|subject| subject.merge_request_id == request.merge_request_id) + .unwrap(); + let sequence = request.thread.len() as u64 + 1; + let requested = ReviewRequestedEvent { + event_id: format!("request-{event_id}"), + sequence, + subject_ref: subject.subject_ref.clone(), + ticket_item_revision: revision.into(), + ticket_merge_request_subjects: snapshot.to_vec(), + requested_by: actor(), + reviewer: actor(), + created_at: request.created_at, + }; + let review = ReviewEvent { + event_id: event_id.into(), + sequence: sequence + 1, + request_event_id: requested.event_id.clone(), + subject_ref: requested.subject_ref.clone(), + ticket_item_revision: revision.into(), + ticket_merge_request_subjects: snapshot.to_vec(), + decision, + body: String::new(), + findings: vec![], + reviewer: actor(), + created_at: request.created_at, + }; + request + .thread + .push(MergeRequestThreadEvent::ReviewRequested(requested)); + request.thread.push(MergeRequestThreadEvent::Review(review)); +} + +fn merged_requests() -> Vec { + let now = Utc.with_ymd_and_hms(2026, 10, 8, 12, 0, 0).unwrap(); + let snapshot = subjects(); + snapshot + .iter() + .map(|subject| { + let mut request = MergeRequest { + workspace_id: "W".into(), + merge_request_id: subject.merge_request_id.clone(), + repository_id: subject.merge_request_id.clone(), + state: MergeRequestState::Merged, + selector_from: Some("work/source".into()), + selector_to: "main".into(), + ticket_ids: vec!["T".into()], + created_at: now, + updated_at: now, + thread: vec![], + }; + let approval_id = format!("integration-{}", subject.merge_request_id); + add_review( + &mut request, + &approval_id, + "revision-one", + &snapshot, + ReviewDecision::Approve, + ); + request + .thread + .push(MergeRequestThreadEvent::Merge(MergeEvent { + event_id: format!("merge-{}", subject.merge_request_id), + sequence: 3, + operation_id: format!("operation-{}", subject.merge_request_id), + approval_event_id: approval_id, + approved_source_ref: subject.subject_ref.clone(), + target_ref_before: "target-before".into(), + target_ref_after: "target-after".into(), + strategy: MergeStrategy::FastForward, + resolution: ConflictResolution::None, + merged_by: actor(), + created_at: now, + })); + request + }) + .collect() +} + +#[test] +fn postmerge_latest_approval_attests_the_current_revision_without_replacing_integration() { + let mut requests = merged_requests(); + let snapshot = subjects(); + assert_eq!( + requirement_approval( + &requests, + "revision-two", + &snapshot, + Some("integration-MR-1") + ), + Err(MergeRequestEvidenceError::ItemRevisionMismatch) + ); + add_review( + &mut requests[0], + "postmerge", + "revision-two", + &snapshot, + ReviewDecision::Approve, + ); + + assert_eq!( + requests[0].integration_approval().unwrap().event_id, + "integration-MR-1" + ); + assert_eq!( + requirement_approval(&requests, "revision-two", &snapshot, None) + .unwrap() + .event_id, + "postmerge" + ); + assert_eq!( + requirement_approval(&requests, "revision-two", &snapshot, Some("postmerge")) + .unwrap() + .event_id, + "postmerge" + ); + assert_eq!( + requirement_approval( + &requests, + "revision-one", + &snapshot, + Some("integration-MR-1") + ), + Err(MergeRequestEvidenceError::ApprovalNotEffective) + ); + assert_eq!( + requirement_approval(&requests, "revision-two", &snapshot, Some("unknown")), + Err(MergeRequestEvidenceError::RequirementApprovalMissing) + ); +} + +#[test] +fn later_request_changes_prevents_older_requirement_approval_from_being_effective() { + let mut requests = merged_requests(); + let snapshot = subjects(); + add_review( + &mut requests[0], + "approved", + "revision-two", + &snapshot, + ReviewDecision::Approve, + ); + add_review( + &mut requests[0], + "changes", + "revision-two", + &snapshot, + ReviewDecision::RequestChanges, + ); + assert_eq!( + requirement_approval(&requests, "revision-two", &snapshot, None), + Err(MergeRequestEvidenceError::RequirementApprovalMissing) + ); + assert_eq!( + requirement_approval(&requests, "revision-two", &snapshot, Some("approved")), + Err(MergeRequestEvidenceError::ApprovalNotEffective) + ); + assert_eq!( + requirement_approval(&requests, "revision-two", &snapshot, Some("changes")), + Err(MergeRequestEvidenceError::ApprovalNotApproved) + ); + assert!(requests[0].integration_approval().is_ok()); +} + +#[test] +fn changed_revision_or_any_linked_source_invalidates_a_multiple_mr_attestation() { + let requests = merged_requests(); + let original = subjects(); + assert!(requirement_approval(&requests, "revision-one", &original, None).is_ok()); + assert_eq!( + requirement_approval( + &requests, + "revision-two", + &original, + Some("integration-MR-1") + ), + Err(MergeRequestEvidenceError::ItemRevisionMismatch) + ); + let mut changed = original.clone(); + changed[1].subject_ref = "changed-source-two".into(); + assert_eq!( + requirement_approval( + &requests, + "revision-one", + &changed, + Some("integration-MR-1") + ), + Err(MergeRequestEvidenceError::SourceSnapshotMismatch) + ); + let mut reversed = original.clone(); + reversed.reverse(); + assert!(requirement_approval(&requests, "revision-one", &reversed, None).is_ok()); + + // Even after the caller refreshes the linked request collection, an old + // attestation cannot authorize completion against a smaller linked set. + assert_eq!( + requirement_approval( + &requests[..1], + "revision-one", + &original[..1], + Some("integration-MR-1") + ), + Err(MergeRequestEvidenceError::SourceSnapshotMismatch) + ); + assert_eq!( + requirement_approval(&requests, "revision-one", &original[..1], None), + Err(MergeRequestEvidenceError::SourceSnapshotMismatch) + ); + assert_eq!( + requirement_approval( + &requests, + "revision-one", + &[original[0].clone(), original[0].clone()], + None + ), + Err(MergeRequestEvidenceError::SourceSnapshotMismatch) + ); +} + +#[test] +fn refreshed_postmerge_approval_can_attest_a_changed_multiple_mr_set_and_revision() { + let mut requests = merged_requests(); + let mut snapshot = subjects(); + snapshot.pop(); + requests.pop(); + assert_eq!( + requirement_approval(&requests, "revision-two", &snapshot, None), + Err(MergeRequestEvidenceError::RequirementApprovalMissing) + ); + add_review( + &mut requests[0], + "refreshed-set", + "revision-two", + &snapshot, + ReviewDecision::Approve, + ); + assert_eq!( + requirement_approval(&requests, "revision-two", &snapshot, None) + .unwrap() + .event_id, + "refreshed-set" + ); + assert_eq!( + requests[0].integration_approval().unwrap().event_id, + "integration-MR-1" + ); +} + +#[test] +fn requirement_approval_must_bind_its_own_mr_source_and_corresponding_request() { + let original = subjects(); + for (case, expected) in [ + ("missing", MergeRequestEvidenceError::ReviewRequestMissing), + ("revision", MergeRequestEvidenceError::ReviewRequestMismatch), + ("snapshot", MergeRequestEvidenceError::ReviewRequestMismatch), + ("reviewer", MergeRequestEvidenceError::ReviewRequestMismatch), + ("source", MergeRequestEvidenceError::ApprovalSourceMismatch), + ] { + let mut requests = merged_requests(); + for event in &mut requests[0].thread { + match event { + MergeRequestThreadEvent::Review(review) if case == "missing" => { + review.request_event_id = "absent-request".into(); + } + MergeRequestThreadEvent::ReviewRequested(requested) => match case { + "revision" => requested.ticket_item_revision = "other-revision".into(), + "snapshot" => { + requested.ticket_merge_request_subjects.pop(); + } + "reviewer" => requested.reviewer.worker_id = "other-reviewer".into(), + "source" => requested.subject_ref = "unbound-source".into(), + _ => {} + }, + MergeRequestThreadEvent::Review(review) if case == "source" => { + review.subject_ref = "unbound-source".into(); + } + _ => {} + } + } + assert_eq!( + requirement_approval( + &requests, + "revision-one", + &original, + Some("integration-MR-1") + ), + Err(expected), + "{case}" + ); + } +} + +#[test] +fn revoked_requirement_approval_is_rejected_and_discovery_uses_another_valid_mr() { + let mut requests = merged_requests(); + let created_at = requests[0].created_at; + requests[0] + .thread + .push(MergeRequestThreadEvent::ReviewRevoked(ReviewRevokedEvent { + event_id: "revocation".into(), + sequence: 4, + review_event_id: "integration-MR-1".into(), + subject_ref: "source-one".into(), + reason: String::new(), + revoked_by: actor(), + created_at, + })); + assert_eq!( + requirement_approval( + &requests, + "revision-one", + &subjects(), + Some("integration-MR-1") + ), + Err(MergeRequestEvidenceError::ApprovalRevoked) + ); + assert_eq!( + requirement_approval(&requests, "revision-one", &subjects(), None) + .unwrap() + .event_id, + "integration-MR-2" + ); +} + +#[test] +fn integration_approval_must_strictly_predate_the_merge_sequence() { + for approval_sequence in [2, 3, 4] { + let mut requests = merged_requests(); + for event in &mut requests[0].thread { + if let MergeRequestThreadEvent::Review(review) = event { + review.sequence = approval_sequence; + } + } + let result = requests[0].integration_approval(); + if approval_sequence < 3 { + assert!(result.is_ok()); + } else { + assert_eq!( + result, + Err(MergeRequestEvidenceError::ApprovalTimelineMismatch), + "approval sequence {approval_sequence}" + ); + } + } +} + +#[test] +fn blank_saved_source_is_not_valid_integration_evidence_even_when_the_review_matches() { + for source in ["", " \t\n"] { + let mut requests = merged_requests(); + for event in &mut requests[0].thread { + match event { + MergeRequestThreadEvent::ReviewRequested(requested) => { + requested.subject_ref = source.into(); + } + MergeRequestThreadEvent::Review(review) => { + review.subject_ref = source.into(); + } + MergeRequestThreadEvent::Merge(merge) => { + merge.approved_source_ref = source.into(); + } + _ => {} + } + } + assert_eq!( + requests[0].integration_approval(), + Err(MergeRequestEvidenceError::ApprovalSourceMismatch) + ); + } +} + +#[test] +fn evidence_error_codes_are_stable_machine_diagnostics_separate_from_messages() { + for (error, code) in [ + (MergeRequestEvidenceError::NotMerged, "not_merged"), + ( + MergeRequestEvidenceError::MergeResultMissing, + "merge_result_missing", + ), + ( + MergeRequestEvidenceError::ApprovalMissing, + "approval_missing", + ), + ( + MergeRequestEvidenceError::ApprovalRevoked, + "approval_revoked", + ), + ( + MergeRequestEvidenceError::ApprovalNotApproved, + "approval_not_approved", + ), + ( + MergeRequestEvidenceError::ApprovalSourceMismatch, + "approval_source_mismatch", + ), + ( + MergeRequestEvidenceError::ApprovalTimelineMismatch, + "approval_timeline_mismatch", + ), + ( + MergeRequestEvidenceError::ReviewRequestMissing, + "review_request_missing", + ), + ( + MergeRequestEvidenceError::ReviewRequestMismatch, + "review_request_mismatch", + ), + ( + MergeRequestEvidenceError::ApprovalNotEffective, + "approval_not_effective", + ), + ( + MergeRequestEvidenceError::ItemRevisionMismatch, + "item_revision_mismatch", + ), + ( + MergeRequestEvidenceError::SourceSnapshotMismatch, + "source_snapshot_mismatch", + ), + ( + MergeRequestEvidenceError::RequirementApprovalMissing, + "requirement_approval_missing", + ), + ] { + assert_eq!(error.code(), code); + assert_ne!(error.code(), error.as_str()); + } +} diff --git a/crates/merge-request/tests/store.rs b/crates/merge-request/tests/store.rs index b73f656eb..d3da0d8f4 100644 --- a/crates/merge-request/tests/store.rs +++ b/crates/merge-request/tests/store.rs @@ -1098,3 +1098,143 @@ fn partial_integration_retains_ticket_and_assignment_until_guarded_ticket_comple .unwrap(); assert_eq!(replay, completed); } + +#[test] +fn persisted_merged_result_requires_matching_nonrevoked_requested_approval_before_completion() { + for (case, expected) in [ + ( + "merge-missing", + MergeRequestEvidenceError::MergeResultMissing, + ), + ( + "approval-missing", + MergeRequestEvidenceError::ApprovalMissing, + ), + ("revoked", MergeRequestEvidenceError::ApprovalRevoked), + ( + "source-mismatch", + MergeRequestEvidenceError::ApprovalSourceMismatch, + ), + ( + "not-approved", + MergeRequestEvidenceError::ApprovalNotApproved, + ), + ( + "request-missing", + MergeRequestEvidenceError::ReviewRequestMissing, + ), + ( + "request-mismatch", + MergeRequestEvidenceError::ReviewRequestMismatch, + ), + ( + "approval-after-merge", + MergeRequestEvidenceError::ApprovalTimelineMismatch, + ), + ] { + let (dir, store) = fixture(); + open(&store); + let review = approve(&store, "merged-source", "integration-token"); + let merge = store + .complete(CompleteMergeRequest { + merge_request_id: "MR".into(), + ticket_id: "T".into(), + operation_id: "merge".into(), + approval_event_id: review.event_id.clone(), + current_subject_ref: "merged-source".into(), + target_ref_before: "before".into(), + target_ref_after: "after".into(), + strategy: MergeStrategy::FastForward, + resolution: ConflictResolution::None, + auth: auth(), + now: at(5), + }) + .unwrap(); + let persisted = store.get_by_id("W", "MR").unwrap(); + assert_eq!(persisted.merged_result().unwrap(), &merge); + assert_eq!(persisted.integration_approval().unwrap(), &review); + + let connection = Connection::open(dir.path().join("db")).unwrap(); + match case { + "merge-missing" | "approval-missing" | "request-missing" => { + let kind = match case { + "merge-missing" => "merge", + "approval-missing" => "review", + _ => "review_requested", + }; + connection.execute( + "DELETE FROM merge_request_thread_events WHERE workspace_id='W' AND merge_request_id='MR' AND kind=?1", + [kind], + ).unwrap(); + } + "revoked" => { + store + .revoke_review(RevokeMergeRequestReview { + merge_request_id: "MR".into(), + ticket_id: "T".into(), + review_event_id: review.event_id.clone(), + reason: "invalid integration evidence".into(), + auth: auth(), + now: at(6), + }) + .unwrap(); + } + "approval-after-merge" => { + let mut altered = review.clone(); + altered.sequence = merge.sequence + 1; + connection.execute( + "UPDATE merge_request_thread_events SET payload_json=?1,sequence=?2 WHERE workspace_id='W' AND merge_request_id='MR' AND kind='review'", + rusqlite::params![serde_json::to_string(&altered).unwrap(), altered.sequence], + ).unwrap(); + } + _ => { + let mut altered = review.clone(); + match case { + "source-mismatch" => altered.subject_ref = "different-source".into(), + "not-approved" => altered.decision = ReviewDecision::RequestChanges, + "request-mismatch" => { + altered.ticket_item_revision = "different-revision".into() + } + _ => unreachable!(), + } + connection.execute( + "UPDATE merge_request_thread_events SET payload_json=?1 WHERE workspace_id='W' AND merge_request_id='MR' AND kind='review'", + [serde_json::to_string(&altered).unwrap()], + ).unwrap(); + } + } + let reloaded = store.get_by_id("W", "MR").unwrap(); + assert_eq!(reloaded.integration_approval(), Err(expected), "{case}"); + let rejected = store + .complete_ticket(CompleteTicket { + ticket_id: "T".into(), + operation_id: "complete-ticket".into(), + item_revision: "t".into(), + merge_request_ids: vec!["MR".into()], + requirement_approval_event_id: review.event_id.clone(), + auth: auth(), + now: at(7), + }) + .unwrap_err(); + match rejected { + MergeRequestError::Corrupt(message) + if expected == MergeRequestEvidenceError::MergeResultMissing => + { + assert!(message.contains(expected.as_str()), "{case}: {message}"); + } + MergeRequestError::NotReady(message) + if expected != MergeRequestEvidenceError::MergeResultMissing => + { + assert!(message.contains(expected.as_str()), "{case}: {message}"); + } + other => panic!("{case}: unexpected completion error {other}"), + } + let (state, assigned): (String, bool) = connection.query_row( + "SELECT workflow_state, EXISTS(SELECT 1 FROM ticket_current_worker_assignments WHERE workspace_id='W' AND ticket_id='T') FROM typed_tickets WHERE workspace_id='W' AND ticket_id='T'", + [], + |row| Ok((row.get(0)?, row.get(1)?)), + ).unwrap(); + assert_eq!(state, "inprogress", "{case}"); + assert!(assigned, "{case}"); + } +}