diff --git a/src-tauri/src/commands/pr.rs b/src-tauri/src/commands/pr.rs index 87ea44d..73d044a 100644 --- a/src-tauri/src/commands/pr.rs +++ b/src-tauri/src/commands/pr.rs @@ -2093,12 +2093,7 @@ async fn settle_delivery( plan: &mut PrReviewPlan, ) -> SettledDelivery { let branch = task.branch_name.as_deref(); - let candidates: Vec = plan.items.iter().enumerate() - .filter(|(_, i)| { - i.approved && matches!(i.decision, PrReviewDecision::Fix) && i.fix_done - }) - .map(|(idx, _)| idx) - .collect(); + let candidates = delivery_candidates(plan); let with_commit = candidates.iter() .filter(|&&idx| plan.items[idx].fix_commit.is_some()) .count(); @@ -2147,7 +2142,38 @@ async fn settle_delivery( } let remote = crate::worktree::delivery::RemoteBranch::refresh(working_dir, Some(branch)).await; settled.unavailable = remote.unavailable().map(str::to_string); - for &idx in &candidates { + settled.indices = delivered_against_remote(working_dir, branch, plan, &remote, &candidates).await; + settled.owed_delivery = owing(plan, &settled.indices); + settled +} + +/// The approved Fix items a delivery check may settle: made (`fix_done`) and +/// approved. Shared by [`settle_delivery`] and the settling of a verified +/// publication, so both ask the same question of the same items. +fn delivery_candidates(plan: &PrReviewPlan) -> Vec { + plan.items.iter().enumerate() + .filter(|(_, i)| { + i.approved && matches!(i.decision, PrReviewDecision::Fix) && i.fix_done + }) + .map(|(idx, _)| idx) + .collect() +} + +/// The per-item rule of [`settle_delivery`], against a remote branch already +/// read: which of `candidates` have their commit in `remote` with their +/// recorded effect still in its tip. The plan's `fix_uncommitted` (and, for a +/// fix that can never be proven, `fix_commit`/`fix_effect`) is updated as +/// [`settle_delivery`] documents. Reads Git only; the caller decides whether +/// the remote branch speaks for the pull request. +async fn delivered_against_remote( + working_dir: &str, + branch: &str, + plan: &mut PrReviewPlan, + remote: &crate::worktree::delivery::RemoteBranch, + candidates: &[usize], +) -> std::collections::HashSet { + let mut delivered = std::collections::HashSet::new(); + for &idx in candidates { let Some(commit) = plan.items[idx].fix_commit.clone() else { continue }; let on_remote = remote.contains(&commit).await; // Holding the commit is not enough: a later commit on the branch @@ -2164,7 +2190,7 @@ async fn settle_delivery( }; if on_remote && verdict == Survival::Present { plan.items[idx].fix_uncommitted = false; - settled.indices.insert(idx); + delivered.insert(idx); continue; } plan.items[idx].fix_uncommitted = true; @@ -2189,8 +2215,7 @@ async fn settle_delivery( plan.items[idx].fix_effect = None; } } - settled.owed_delivery = owing(plan, &settled.indices); - settled + delivered } /// Result of `sync_pr_review_replies` — counts the three operations Sync can diff --git a/src-tauri/src/commands/pr/publication.rs b/src-tauri/src/commands/pr/publication.rs index b33d646..4a38f34 100644 --- a/src-tauri/src/commands/pr/publication.rs +++ b/src-tauri/src/commands/pr/publication.rs @@ -14,11 +14,25 @@ //! was. A push whose outcome cannot be read back is recorded as unknown, and //! the next request reads the remote again before it may push. +//! Review fixes. A publication that is recorded as `Published` is also an +//! observation of the remote branch, so it settles the Task's PR-review plan: +//! a fix whose commit that read-back branch holds, with its recorded effect +//! still in the tip, stops being marked as not yet on the remote +//! (`fix_uncommitted`). It is the rule Apply and Sync replies use, fed with the +//! branch this request just read, and it is written in the same board write as +//! the `Published` record. It only ever clears that marker. It posts no reply, +//! changes no other field, and is skipped for every outcome that is not +//! `Published`. A repeat answered from an existing record reads nothing and +//! settles nothing. +//! //! Residual limits. The pull request is read again after the record is //! written and before the push, but GitHub can still merge or close it in the //! moments between that read and the push, and the push then reaches the -//! branch. A repeated request for a commit already recorded as published is -//! answered from the record, without reading the remote again. A run that +//! branch. A repeated request for a commit already recorded as published +//! from the same branch to the same pull request is answered from the record, +//! without reading the remote again. A record of another branch or pull request +//! never answers it: a pending one is settled for its own operation first, and +//! the request is then refused for the person to confirm again. A run that //! starts after the live-run check and before the reservation is not refused. use std::sync::atomic::{AtomicBool, Ordering}; @@ -27,7 +41,8 @@ use uuid::Uuid; use crate::commands::gui_admission::GuiMutation; use crate::commands::pr::{ - own_pr_url_for_branch, pr_url_for_task, push_branch_pinned_gated, refuse_if_pr_operation_cancelled, + delivered_against_remote, delivery_candidates, own_pr_url_for_branch, pr_url_for_task, + push_branch_pinned_gated, refuse_if_pr_operation_cancelled, reserve_task_for_explicit_publication, resolve_repository_dir, run_cmd, task_branch_tip, }; use crate::domain::publication::{ @@ -57,10 +72,10 @@ use crate::worktree::restack; /// `Err` means this call returned no record. It covers four cases: the request /// could not start (a running task, a pull request that is not this task's, an /// unreadable repository); a record write did not land before any push; an -/// earlier publication of a different commit or pull request was still pending -/// or unknown; and a push ran but its outcome could not be recorded, in which -/// case the error text says whether the push succeeded. In the third case that -/// earlier record is reconciled against the remote and its settled state is +/// earlier publication of a different commit, branch or pull request was +/// still pending or unknown; and a push ran but its outcome could not be +/// recorded, in which case the error text says whether the push succeeded. In the +/// third case that earlier record is reconciled against the remote and its settled state is /// written, so the stored record can change even though the request returns /// `Err`. A request pushes at most the commit it was given, and only after its /// pending record was written. @@ -178,6 +193,43 @@ mod test_seam { } } +/// A review fix the remote branch, as this request read it, provably holds. +struct DeliveredFix { + /// The item's place in the plan, and what identifies it there. The write + /// settles it only if the plan still has the same item and commit. + index: usize, + comment_id: Option, + commit: String, +} + +/// The review fixes of the Task's plan that `remote` provably holds, by the +/// rule Apply and Sync replies use. Empty when the Task has no plan, no branch, +/// or nothing the remote holds. Reads Git only; the plan is worked on as a copy. +async fn delivered_review_fixes( + state: &crate::AppState, + task_id: Uuid, + working_dir: &str, + remote: &RemoteBranch, +) -> Vec { + let Some(task) = state.task.tasks.read().await.get(&task_id).cloned() else { + return Vec::new(); + }; + let (Some(branch), Some(mut plan)) = (task.branch_name.as_deref(), task.pr_review_plan.clone()) else { + return Vec::new(); + }; + let candidates = delivery_candidates(&plan); + let delivered = delivered_against_remote(working_dir, branch, &mut plan, remote, &candidates).await; + let mut fixes: Vec = delivered + .into_iter() + .filter_map(|index| { + let item = &plan.items[index]; + Some(DeliveredFix { index, comment_id: item.comment_id, commit: item.fix_commit.clone()? }) + }) + .collect(); + fixes.sort_by_key(|f| f.index); + fixes +} + /// Write `record` as the task's publication, but only while the task still /// holds `expected` (compare-and-set). `Err` says whether the write landed: /// `NotPublished` means the board is unchanged, including when `expected` no @@ -188,6 +240,20 @@ async fn write_publication( task_id: Uuid, expected: Option<&PrPublication>, record: &PrPublication, +) -> Result<(), RecordWriteError> { + write_publication_settling(state, task_id, expected, record, &[]).await +} + +/// [`write_publication`], which also clears the not-yet-on-the-remote marker of +/// the review `delivered` fixes in the same board write, so a restart finds the +/// record and the plan agreeing. A fix whose item or commit changed since it +/// was observed is left as it is. +async fn write_publication_settling( + state: &crate::AppState, + task_id: Uuid, + expected: Option<&PrPublication>, + record: &PrPublication, + delivered: &[DeliveredFix], ) -> Result<(), RecordWriteError> { let recorded = AtomicBool::new(false); let apply = |staged: &mut std::collections::HashMap| { @@ -195,6 +261,18 @@ async fn write_publication( if task.pr_publication.as_ref() == expected { task.pr_publication = Some(record.clone()); recorded.store(true, Ordering::SeqCst); + if let Some(plan) = task.pr_review_plan.as_mut() { + for fix in delivered { + if let Some(item) = plan.items.get_mut(fix.index) { + if item.fix_done + && item.comment_id == fix.comment_id + && item.fix_commit.as_deref() == Some(fix.commit.as_str()) + { + item.fix_uncommitted = false; + } + } + } + } } } }; @@ -289,7 +367,7 @@ async fn publish( let working_dir = resolve_repository_dir(state, task_id).await?; let existing = task.pr_publication.clone(); - match replay(existing.as_ref(), commit, pr_url) { + match replay(existing.as_ref(), commit, &branch, pr_url) { Replay::AlreadyPublished => return Ok(existing.expect("replay names an existing record")), Replay::Start => {} Replay::MustReconcile => { @@ -308,17 +386,31 @@ async fn publish( ); }; let updated = PrPublication { state: state_now, updated_at: chrono::Utc::now(), ..record.clone() }; - write_publication(state, task_id, Some(&record), &updated) + // The remote was just read: when it holds the record's commit, it + // also settles the review fixes it carries, but only for a record + // of this branch and pull request: another branch's remote says + // nothing about this Task's plan. + let delivered = if updated.state == (PublicationState::Published {}) + && record.branch == branch + && record.pr_url == pr_url + { + delivered_review_fixes(state, task_id, &working_dir, &remote).await + } else { + Vec::new() + }; + write_publication_settling(state, task_id, Some(&record), &updated, &delivered) .await .map_err(record_write_failure)?; - if updated.commit != commit || updated.pr_url != pr_url { - // The stored record was for another commit or another pull request. + if updated.commit != commit || updated.branch != branch || updated.pr_url != pr_url { + // The stored record was for another commit, branch or pull request. // Its outcome is now settled, but this request was not pushed, and // reconciling must not start a push for it. return Err(format!( - "An earlier publication of {} to {} was settled against the remote. The commit you confirmed, \ - {}, was not published, and nothing was pushed for it. Confirm the publication again to publish it.", + "An earlier publication of {} from {} to {} was settled against the remote. The commit you \ + confirmed, {}, was not published, and nothing was pushed for it. Confirm the publication \ + again to publish it.", short_commit(&record.commit), + record.branch, record.pr_url, short_commit(commit), )); @@ -417,12 +509,20 @@ async fn publish( write_publication(state, task_id, Some(&pending), &record).await.map_err(record_write_failure)?; return Ok(record); } - let remote_after = remote_head_of(&RemoteBranch::refresh(&working_dir, Some(branch.as_str())).await); + let remote_read = RemoteBranch::refresh(&working_dir, Some(branch.as_str())).await; + let remote_after = remote_head_of(&remote_read); let settled = classify_outcome(commit, remote_head.as_deref(), &remote_after, &push); + // Only a read-back that found the commit is evidence for the review fixes. + // A refusal, a pending or an unknown outcome proves nothing about them. + let delivered = if settled == (PublicationState::Published {}) { + delivered_review_fixes(state, task_id, &working_dir, &remote_read).await + } else { + Vec::new() + }; let outcome = if push.is_ok() { "succeeded" } else { "failed" }; let finished = PrPublication { state: settled, updated_at: chrono::Utc::now(), ..record }; - write_publication(state, task_id, Some(&pending), &finished).await.map_err(|e| match e { + write_publication_settling(state, task_id, Some(&pending), &finished, &delivered).await.map_err(|e| match e { // Nothing changed on disk or in memory: the record still says pending. RecordWriteError::NotPublished(_) => format!( "The push {outcome}, but its outcome could not be recorded: {e}. The record still says pending, so \ @@ -466,9 +566,10 @@ mod orchestration { use super::{publish, test_seam}; use crate::domain::publication::{PrPublication, PublicationState}; - use crate::domain::task::HumanReviewRecord; + use crate::domain::task::{FixEffect, HumanReviewRecord, PrReviewDecision, PrReviewItem, PrReviewPlan}; use crate::domain::{AgentConfig, AgentType, Project, ProjectScope, Repository, Task, TaskStatus}; use crate::test_helpers::create_test_task_full; + use crate::worktree::delivery; use std::collections::HashMap; use std::path::{Path, PathBuf}; use uuid::Uuid; @@ -617,6 +718,69 @@ mod orchestration { (!listed.is_empty()).then_some(listed) } + /// What a fix agent would have done: change `name` in the checkout, + /// recording the trees before and after, then commit it as the fix's + /// commit. Returns that commit and the effect. + async fn fix_committed(&self, name: &str) -> (String, FixEffect) { + git(&self.checkout, &["checkout", "-q", BRANCH]); + let dir = self.checkout.to_str().unwrap(); + let before_tree = delivery::checkout_snapshot(dir).await.unwrap(); + std::fs::write(self.checkout.join(name), format!("fix for {name}\n")).unwrap(); + let after_tree = delivery::checkout_snapshot(dir).await.unwrap(); + git(&self.checkout, &["add", name]); + git(&self.checkout, &["commit", "-q", "-m", &format!("fix {name}")]); + (git(&self.checkout, &["rev-parse", "HEAD"]), FixEffect { before_tree, after_tree }) + } + + /// Give the Task a review plan whose one approved Fix item was made, + /// committed (`commit`) and not yet seen on the remote, as a first + /// Apply with Auto-push off leaves it. + async fn seed_plan_with_unpublished_fix(&self, commit: &str, effect: FixEffect) { + let item = PrReviewItem { + comment_id: Some(7), + summary: "rename".into(), + decision: PrReviewDecision::Fix, + reasoning: String::new(), + proposed_change: String::new(), + approved: true, + user_note: String::new(), + fix_done: true, + fix_uncommitted: true, + fix_commit: Some(commit.to_string()), + fix_effect: Some(effect), + reply_posted: false, + last_agent_summary: Some("done".into()), + last_error: None, + pr_reply_text: None, + reply_comment_id: None, + }; + let plan = PrReviewPlan { + generated_at: chrono::Utc::now(), + pr_url: PR.to_string(), + review_decision: None, + comments: Vec::new(), + items: vec![item], + raw_plan: String::new(), + last_apply: None, + fixed_content: Vec::new(), + }; + let mut tasks = self.state.task.tasks.write().await; + tasks.get_mut(&self.task_id).unwrap().pr_review_plan = Some(plan); + let board: Vec = tasks.values().cloned().collect(); + self.state.storage.save_project_tasks(self.project_id, &board).unwrap(); + } + + async fn plan_item(&self) -> PrReviewItem { + self.task().await.pr_review_plan.expect("the task keeps its plan").items.remove(0) + } + + /// The plan item as the file holds it, for what a restart would find. + fn persisted_plan_item(&self) -> PrReviewItem { + let board = self.state.storage.load_project_tasks(self.project_id).unwrap(); + let task = board.into_iter().find(|t| t.id == self.task_id).unwrap(); + task.pr_review_plan.expect("the plan is on disk").items.remove(0) + } + async fn task(&self) -> Task { self.state.task.tasks.read().await.get(&self.task_id).cloned().unwrap() } @@ -901,6 +1065,88 @@ mod orchestration { assert_eq!(fx.remote_head().as_deref(), Some(c1.as_str())); } + // A record answers a request only for the branch, commit and pull request + // the person confirmed. Another branch's record, pending or published, is + // settled for its own operation and never answers or authorizes this one. + + #[tokio::test] + async fn a_published_record_for_another_branch_does_not_answer_a_request_for_this_branch() { + let fx = fixture().await; + let c1 = fx.commit("one"); + let other = "other-branch"; + let earlier = PrPublication { state: PublicationState::Published {}, branch: other.to_string(), ..pending_record(&c1, &fx.base) }; + fx.leave_record(earlier.clone()).await; + test_seam::fix_observation(true, BRANCH, true); + + let record = publish(&fx.state, fx.task_id, BRANCH, &c1, PR).await.unwrap(); + + assert_eq!(record.branch, BRANCH, "the answer is for the branch that was asked for"); + assert_ne!(record.request_id, earlier.request_id, "the other branch's record is not the answer"); + assert_eq!(record.state, PublicationState::Published {}); + assert_eq!(fx.remote_head().as_deref(), Some(c1.as_str()), "this request, which the person confirmed, ran on its own"); + let task = fx.task().await; + assert_eq!(task.human_review, HumanReviewRecord::default(), "publication must not record a decision"); + assert_eq!(task.status, TaskStatus::HumanReview); + } + + #[tokio::test] + async fn a_settled_record_for_the_same_commit_and_pull_request_on_another_branch_is_not_reported_as_published() { + let fx = fixture().await; + let c1 = fx.commit("one"); + // c1 reached another branch of the remote, whose earlier publication is still pending. + let other = "other-branch"; + git(&fx.checkout, &["push", "-q", "origin", &format!("{c1}:refs/heads/{other}")]); + fx.leave_record(PrPublication { branch: other.to_string(), ..pending_record(&c1, &fx.base) }).await; + test_seam::fix_observation(true, BRANCH, true); + + let err = publish(&fx.state, fx.task_id, BRANCH, &c1, PR).await.unwrap_err(); + + assert!(err.contains("was not published"), "{err}"); + let settled = fx.record().await.unwrap(); + assert_eq!((settled.branch.as_str(), settled.state), (other, PublicationState::Published {})); + assert_eq!(fx.remote_head().as_deref(), Some(fx.base.as_str()), "reconciling another branch must not push this one"); + } + + #[tokio::test] + async fn a_refused_settlement_of_another_branch_record_does_not_answer_or_publish_this_request() { + let fx = fixture().await; + let c1 = fx.commit("one"); + // The earlier publication for another branch never landed: its remote lacks c1. + let other = "other-branch"; + git(&fx.checkout, &["push", "-q", "origin", &format!("{}:refs/heads/{other}", fx.base)]); + fx.leave_record(PrPublication { branch: other.to_string(), ..pending_record(&c1, &fx.base) }).await; + test_seam::fix_observation(true, BRANCH, true); + + let err = publish(&fx.state, fx.task_id, BRANCH, &c1, PR).await.unwrap_err(); + + assert!(err.contains("was not published"), "{err}"); + let settled = fx.record().await.unwrap(); + assert_eq!(settled.branch, other); + assert!(matches!(settled.state, PublicationState::Refused { .. })); + assert_eq!(fx.remote_head().as_deref(), Some(fx.base.as_str()), "settling the old record pushed nothing"); + } + + #[tokio::test] + async fn an_unknown_record_for_another_branch_stays_unknown_while_its_remote_cannot_be_read() { + let fx = fixture().await; + let c1 = fx.commit("one"); + let other = "other-branch"; + let earlier = PrPublication { + state: PublicationState::Unknown { reason: "test".into() }, + branch: other.to_string(), + ..pending_record(&c1, &fx.base) + }; + fx.leave_record(earlier.clone()).await; + test_seam::fix_observation(true, BRANCH, true); + git(&fx.checkout, &["remote", "set-url", "origin", "/nonexistent/remote.git"]); + + let err = publish(&fx.state, fx.task_id, BRANCH, &c1, PR).await.unwrap_err(); + + assert!(err.contains("still unknown"), "{err}"); + assert_eq!(fx.record().await, Some(earlier), "an unreadable remote settles nothing"); + assert_eq!(fx.remote_head().as_deref(), Some(fx.base.as_str())); + } + #[tokio::test] async fn a_failed_pending_write_pushes_nothing() { let fx = fixture().await; @@ -1118,4 +1364,187 @@ mod orchestration { assert!(fx.record().await.is_none(), "a refused request records nothing"); assert_eq!(fx.remote_head().as_deref(), Some(fx.base.as_str())); } + + // ----- The review plan after a publication ----------------------------- + // + // Publish to PR pushes the commit a review fix was committed in. The plan + // marks that fix "not delivered" until something has seen it on the remote, + // and Sync replies stays unavailable meanwhile. A verified publication is + // such an observation, so it settles the plan; nothing else about the plan + // changes, and no reply is posted. + + #[tokio::test] + async fn a_verified_publication_of_the_fix_commit_settles_the_plan_without_a_reply() { + let fx = fixture().await; + let (c1, effect) = fx.fix_committed("fixed.txt").await; + fx.seed_plan_with_unpublished_fix(&c1, effect.clone()).await; + test_seam::fix_observation(true, BRANCH, true); + let before = fx.plan_item().await; + assert!(before.fix_uncommitted, "precondition: the fix is not yet seen on the remote"); + + let record = publish(&fx.state, fx.task_id, BRANCH, &c1, PR).await.unwrap(); + + assert_eq!(record.state, PublicationState::Published {}); + assert_eq!(fx.remote_head().as_deref(), Some(c1.as_str())); + let item = fx.plan_item().await; + assert!(!item.fix_uncommitted, "the remote holds the fix commit and its effect"); + assert_eq!(item, PrReviewItem { fix_uncommitted: false, ..before }, "nothing else on the item changes"); + assert!(!item.reply_posted && item.reply_comment_id.is_none(), "publishing posts no reply"); + assert_eq!(fx.persisted_plan_item(), item, "the file holds what memory holds, so a restart agrees"); + // The one thing Sync's availability asks of the plan. + assert!(item.fix_done && item.fix_commit.is_some() && !item.fix_uncommitted); + assert_eq!(item.fix_effect, Some(effect)); + } + + #[tokio::test] + async fn a_publication_of_a_later_commit_settles_a_fix_it_contains() { + let fx = fixture().await; + let (c1, effect) = fx.fix_committed("fixed.txt").await; + fx.seed_plan_with_unpublished_fix(&c1, effect).await; + let c2 = fx.commit("unrelated"); + test_seam::fix_observation(true, BRANCH, true); + + let record = publish(&fx.state, fx.task_id, BRANCH, &c2, PR).await.unwrap(); + + assert_eq!(record.state, PublicationState::Published {}); + assert!(!fx.plan_item().await.fix_uncommitted, "c1 is an ancestor of the published c2"); + } + + #[tokio::test] + async fn a_refused_publication_never_settles_the_plan() { + let fx = fixture().await; + let (c1, effect) = fx.fix_committed("fixed.txt").await; + fx.seed_plan_with_unpublished_fix(&c1, effect).await; + test_seam::fix_observation(false, BRANCH, true); + + let record = publish(&fx.state, fx.task_id, BRANCH, &c1, PR).await.unwrap(); + + assert!(matches!(record.state, PublicationState::Refused { .. })); + assert!(fx.plan_item().await.fix_uncommitted, "an attempt is not delivery"); + assert!(fx.persisted_plan_item().fix_uncommitted); + } + + #[tokio::test] + async fn a_published_commit_that_reverted_the_fix_does_not_settle_it() { + let fx = fixture().await; + let (c1, effect) = fx.fix_committed("fixed.txt").await; + fx.seed_plan_with_unpublished_fix(&c1, effect).await; + // The branch tip removes the fix again. + git(&fx.checkout, &["rm", "-q", "fixed.txt"]); + git(&fx.checkout, &["commit", "-q", "-m", "revert the fix"]); + let tip = git(&fx.checkout, &["rev-parse", "HEAD"]); + test_seam::fix_observation(true, BRANCH, true); + + let record = publish(&fx.state, fx.task_id, BRANCH, &tip, PR).await.unwrap(); + + assert_eq!(record.state, PublicationState::Published {}); + let item = fx.plan_item().await; + assert!(item.fix_uncommitted, "the remote holds the commit but not the change"); + assert_eq!(item.fix_commit.as_deref(), Some(c1.as_str()), "publishing never discards the fix's evidence"); + assert!(item.fix_effect.is_some()); + } + + #[tokio::test] + async fn a_fix_that_was_never_committed_stays_undelivered_after_a_publication() { + let fx = fixture().await; + let (_c1, effect) = fx.fix_committed("fixed.txt").await; + let tip = fx.commit("other"); + fx.seed_plan_with_unpublished_fix("0000000000000000000000000000000000000001", effect).await; + test_seam::fix_observation(true, BRANCH, true); + + let record = publish(&fx.state, fx.task_id, BRANCH, &tip, PR).await.unwrap(); + + assert_eq!(record.state, PublicationState::Published {}); + assert!(fx.plan_item().await.fix_uncommitted, "a commit the remote does not hold proves nothing"); + } + + #[tokio::test] + async fn repeating_a_published_request_changes_nothing_and_posts_no_second_reply() { + let fx = fixture().await; + let (c1, effect) = fx.fix_committed("fixed.txt").await; + fx.seed_plan_with_unpublished_fix(&c1, effect).await; + test_seam::fix_observation(true, BRANCH, true); + let first = publish(&fx.state, fx.task_id, BRANCH, &c1, PR).await.unwrap(); + let settled = fx.task().await; + + let again = publish(&fx.state, fx.task_id, BRANCH, &c1, PR).await.unwrap(); + + assert_eq!(again, first); + let after = fx.task().await; + assert_eq!(after.pr_review_plan, settled.pr_review_plan); + assert_eq!(after.updated_at, settled.updated_at, "a replay writes nothing"); + assert!(!after.pr_review_plan.unwrap().items[0].reply_posted); + } + + #[tokio::test] + async fn a_push_that_landed_before_its_record_settled_also_settles_the_plan() { + let fx = fixture().await; + let (c1, effect) = fx.fix_committed("fixed.txt").await; + fx.seed_plan_with_unpublished_fix(&c1, effect).await; + git(&fx.checkout, &["push", "-q", "origin", &format!("{c1}:refs/heads/{BRANCH}")]); + fx.leave_record(pending_record(&c1, &fx.base)).await; + test_seam::fix_observation(true, BRANCH, true); + + let record = publish(&fx.state, fx.task_id, BRANCH, &c1, PR).await.unwrap(); + + assert_eq!(record.state, PublicationState::Published {}); + assert!(!fx.plan_item().await.fix_uncommitted, "the reconciled record is the same observation"); + } + + // An earlier record for another branch or pull request is settled against + // the remote, but what its remote branch holds says nothing about this + // Task's plan. The record is written as before; the plan is left alone. + + #[tokio::test] + async fn a_reconciled_record_for_another_branch_does_not_settle_the_plan() { + let fx = fixture().await; + let (c1, effect) = fx.fix_committed("fixed.txt").await; + fx.seed_plan_with_unpublished_fix(&c1, effect).await; + // c1 is on a different branch of the remote, whose earlier publication is still pending. + let other = "other-branch"; + git(&fx.checkout, &["push", "-q", "origin", &format!("{c1}:refs/heads/{other}")]); + fx.leave_record(PrPublication { branch: other.to_string(), ..pending_record(&c1, &fx.base) }).await; + test_seam::fix_observation(true, BRANCH, true); + + let err = publish(&fx.state, fx.task_id, BRANCH, &c1, PR).await.unwrap_err(); + + assert!(err.contains("was not published"), "{err}"); + let settled = fx.record().await.unwrap(); + assert_eq!((settled.branch.as_str(), settled.state), (other, PublicationState::Published {})); + assert!(fx.plan_item().await.fix_uncommitted, "another branch's remote does not speak for this plan"); + assert!(fx.persisted_plan_item().fix_uncommitted); + assert_eq!(fx.remote_head().as_deref(), Some(fx.base.as_str()), "this task's branch was not pushed"); + } + + #[tokio::test] + async fn a_reconciled_record_for_another_pull_request_does_not_settle_the_plan() { + let fx = fixture().await; + let (c1, effect) = fx.fix_committed("fixed.txt").await; + fx.seed_plan_with_unpublished_fix(&c1, effect).await; + // c1 reached this task's branch for an earlier pull request; its record is still pending. + git(&fx.checkout, &["push", "-q", "origin", &format!("{c1}:refs/heads/{BRANCH}")]); + let earlier_pr = "https://github.com/BarraDev/slashit/pull/8"; + fx.leave_record(PrPublication { pr_url: earlier_pr.to_string(), ..pending_record(&c1, &fx.base) }).await; + test_seam::fix_observation(true, BRANCH, true); + + let err = publish(&fx.state, fx.task_id, BRANCH, &c1, PR).await.unwrap_err(); + + assert!(err.contains("was not published"), "{err}"); + let settled = fx.record().await.unwrap(); + assert_eq!((settled.pr_url.as_str(), settled.state), (earlier_pr, PublicationState::Published {})); + assert!(fx.plan_item().await.fix_uncommitted, "a record for another pull request settles nothing"); + assert!(fx.persisted_plan_item().fix_uncommitted); + } + + #[tokio::test] + async fn a_task_without_a_review_plan_publishes_as_before() { + let fx = fixture().await; + let c1 = fx.commit("one"); + test_seam::fix_observation(true, BRANCH, true); + + let record = publish(&fx.state, fx.task_id, BRANCH, &c1, PR).await.unwrap(); + + assert_eq!(record.state, PublicationState::Published {}); + assert!(fx.task().await.pr_review_plan.is_none()); + } } diff --git a/src-tauri/src/domain/publication.rs b/src-tauri/src/domain/publication.rs index 472ba89..0a8d2ff 100644 --- a/src-tauri/src/domain/publication.rs +++ b/src-tauri/src/domain/publication.rs @@ -226,21 +226,23 @@ pub fn reconcile( pub enum Replay { /// No usable record: the request may start. Start, - /// The commit is already published to this pull request. Nothing else - /// is pushed, and the existing record answers the request. + /// The commit is already published from this branch to this pull request. + /// Nothing else is pushed, and the existing record answers the request. AlreadyPublished, /// An earlier request must be reconciled first. MustReconcile, } /// Decide what an existing record means for a new request to publish -/// `commit` to `pr_url`. -pub fn replay(existing: Option<&PrPublication>, commit: &str, pr_url: &str) -> Replay { +/// `commit` from `branch` to `pr_url`. A published record answers the request +/// only when it is for the same commit, branch and pull request. +pub fn replay(existing: Option<&PrPublication>, commit: &str, branch: &str, pr_url: &str) -> Replay { match existing { None => Replay::Start, Some(record) if record.needs_reconciliation() => Replay::MustReconcile, Some(record) if record.state == PublicationState::Published {} && record.commit == commit + && record.branch == branch && record.pr_url == pr_url => { Replay::AlreadyPublished @@ -404,24 +406,30 @@ mod tests { #[test] fn a_pending_or_unknown_record_forces_reconciliation_before_a_new_request() { - assert_eq!(replay(Some(&record(PublicationState::Pending {})), NEW, PR), Replay::MustReconcile); + assert_eq!(replay(Some(&record(PublicationState::Pending {})), NEW, BRANCH, PR), Replay::MustReconcile); let unknown = record(PublicationState::Unknown { reason: "x".into() }); - assert_eq!(replay(Some(&unknown), NEW, PR), Replay::MustReconcile); + assert_eq!(replay(Some(&unknown), NEW, BRANCH, PR), Replay::MustReconcile); } #[test] fn a_duplicate_request_for_a_published_commit_is_answered_without_a_push() { let published = record(PublicationState::Published {}); - assert_eq!(replay(Some(&published), NEW, PR), Replay::AlreadyPublished); + assert_eq!(replay(Some(&published), NEW, BRANCH, PR), Replay::AlreadyPublished); // A different commit is a new request. - assert_eq!(replay(Some(&published), OLD, PR), Replay::Start); + assert_eq!(replay(Some(&published), OLD, BRANCH, PR), Replay::Start); + // The same commit and pull request from another branch is a new request. + assert_eq!(replay(Some(&published), NEW, "other-branch", PR), Replay::Start); + assert_eq!( + replay(Some(&published), NEW, BRANCH, "https://github.com/BarraDev/slashit/pull/10"), + Replay::Start + ); } #[test] fn a_refused_record_lets_a_new_request_start() { let refused = record(PublicationState::Refused { reason: "x".into() }); - assert_eq!(replay(Some(&refused), NEW, PR), Replay::Start); - assert_eq!(replay(None, NEW, PR), Replay::Start); + assert_eq!(replay(Some(&refused), NEW, BRANCH, PR), Replay::Start); + assert_eq!(replay(None, NEW, BRANCH, PR), Replay::Start); } #[test] diff --git a/src-tauri/src/domain/task.rs b/src-tauri/src/domain/task.rs index 06042f3..1e24fe5 100644 --- a/src-tauri/src/domain/task.rs +++ b/src-tauri/src/domain/task.rs @@ -1596,9 +1596,9 @@ pub struct PrReviewItem { pub fix_done: bool, /// Bookkeeping for the UI and for what an apply still owes, never /// evidence: true from the moment this item's fix agent succeeds until an - /// apply or a sync has seen the fix delivered (see - /// [`PrReviewItem::fix_commit`] and [`PrReviewItem::fix_effect`]), and set - /// again if it is no longer. Clearing it proves nothing, and a plan + /// apply, a sync or a verified Publish to PR has seen the fix delivered + /// (see [`PrReviewItem::fix_commit`] and [`PrReviewItem::fix_effect`]), and + /// set again by an apply or a sync if it is no longer. Clearing it proves nothing, and a plan /// persisted before commits were recorded has it cleared without any proof /// (#96). Whether a reply may say the fix is done is decided only by /// delivery as the remote shows it at that moment. diff --git a/src/components/pr_publication.rs b/src/components/pr_publication.rs index 9d02f68..0c8c2c8 100644 --- a/src/components/pr_publication.rs +++ b/src/components/pr_publication.rs @@ -168,22 +168,27 @@ pub fn PublishToPrPanel( notice.set(None); spawn_local(async move { let requested = commit.clone(); + let requested_branch = branch.clone(); let requested_pr = pr_url.clone(); let answer = publish_task_to_pr(task_id.to_string(), branch, commit, pr_url).await; let _ = busy.try_set(false); match answer { - Ok(record) if answers_request(&record, &requested, &requested_pr) => { + Ok(record) + if answers_request(&record, &requested, &requested_branch, &requested_pr) => + { let _ = answered.try_set(Some(record)); let _ = confirmation.try_set(None); } Ok(record) => { - // The backend answered for a different commit, for example - // an earlier publication it settled first. Show nothing as - // published for the commit that was asked for. + // The backend answered for a different commit or branch, for + // example an earlier publication it settled first. Show + // nothing as published for the commit that was asked for. let _ = error.try_set(Some(format!( - "The answer was for {}, not {}. Nothing was published for {}. Review the branch and try again.", + "The answer was for {} on {}, not {} on {}. Nothing was published for {}. Review the branch and try again.", short(&record.commit), + record.branch, short(&requested), + requested_branch, short(&requested), ))); } diff --git a/src/components/pr_publication_state.rs b/src/components/pr_publication_state.rs index 31a00b9..e58d7fd 100644 --- a/src/components/pr_publication_state.rs +++ b/src/components/pr_publication_state.rs @@ -38,15 +38,19 @@ pub fn confirmation_still_matches(shown: &PublishConfirmation, live: LiveTarget< && live.pr_url == Some(shown.pr_url.as_str()) } -/// Whether a publication answer is for the commit and pull request the request -/// named. A reconciled earlier record is an answer for another commit or another -/// pull request, and must not be shown or taken as success for the requested one. +/// Whether a publication answer is for the commit, branch and pull request the +/// request named. A reconciled earlier record is an answer for another commit, +/// branch or pull request, and must not be shown or taken as success for the +/// requested one. pub fn answers_request( answer: &PrPublication, requested_commit: &str, + requested_branch: &str, requested_pr_url: &str, ) -> bool { - answer.commit == requested_commit && answer.pr_url == requested_pr_url + answer.commit == requested_commit + && answer.branch == requested_branch + && answer.pr_url == requested_pr_url } /// The record the panel shows: the later of the task's record and the panel's @@ -162,16 +166,27 @@ mod tests { #[test] fn an_answer_counts_only_for_the_commit_that_was_requested() { let reconciled_earlier = record(TIP, 10); - assert!(answers_request(&reconciled_earlier, TIP, PR)); - assert!(!answers_request(&reconciled_earlier, NEXT, PR)); + assert!(answers_request(&reconciled_earlier, TIP, BRANCH, PR)); + assert!(!answers_request(&reconciled_earlier, NEXT, BRANCH, PR)); // The same commit on another pull request is a different request. assert!(!answers_request( &reconciled_earlier, TIP, + BRANCH, "https://github.com/BarraDev/slashit/pull/10" )); } + #[test] + fn an_answer_for_the_same_commit_and_pull_request_on_another_branch_is_not_the_answer() { + let other_branch = PrPublication { + branch: "other-branch".into(), + ..record(TIP, 10) + }; + assert!(!answers_request(&other_branch, TIP, BRANCH, PR)); + assert!(answers_request(&other_branch, TIP, "other-branch", PR)); + } + #[test] fn a_newer_local_answer_is_shown_over_an_older_task_record() { let task = record(TIP, 10);