From 4ab02d73c96b366134cb6c2ec4dd923f09ba8943 Mon Sep 17 00:00:00 2001 From: Rui Andrada <27135+shingonoide@users.noreply.github.com> Date: Sun, 1 Nov 2026 09:21:43 -0300 Subject: [PATCH 1/4] fix(pr): settle a PR-review plan when Publish to PR is verified Publish to PR pushes the commit that carries a review fix, but only Apply and Sync replies ever cleared the plan's "not yet on the remote" marker (fix_uncommitted). After a verified publication the plan kept calling the fix undelivered and Sync replies stayed unavailable until Apply ran again. When a publication is recorded as Published, including one settled by reconciling an earlier pending push, the remote branch it just read now settles the plan with the same per-item rule Apply and Sync use: the fix's commit must be in that branch and its recorded effect still in the tip. The marker is cleared in the same board write as the Published record, so a restart finds both. Refused, pending and unknown outcomes settle nothing, a repeat answered from an existing record reads nothing, and no reply is posted. The per-item delivery rule is split out of settle_delivery so both callers share it. --- src-tauri/src/commands/pr.rs | 45 +++- src-tauri/src/commands/pr/publication.rs | 302 ++++++++++++++++++++++- src-tauri/src/domain/task.rs | 6 +- 3 files changed, 335 insertions(+), 18 deletions(-) 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..960f4ca 100644 --- a/src-tauri/src/commands/pr/publication.rs +++ b/src-tauri/src/commands/pr/publication.rs @@ -14,6 +14,17 @@ //! 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 @@ -27,7 +38,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::{ @@ -178,6 +190,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 +237,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 +258,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; + } + } + } + } } } }; @@ -308,7 +383,14 @@ 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. + let delivered = if updated.state == (PublicationState::Published {}) { + 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 { @@ -417,12 +499,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 +556,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 +708,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() } @@ -1118,4 +1272,142 @@ 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"); + } + + #[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/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. From 9493ad62b175de9f7661f8310f0b8beaeb369dec Mon Sep 17 00:00:00 2001 From: Rui Andrada <27135+shingonoide@users.noreply.github.com> Date: Sun, 1 Nov 2026 09:57:22 -0300 Subject: [PATCH 2/4] fix(pr): settle the review plan on reconcile only for this branch and pull request When a publication request finds an earlier Pending or Unknown record and settles it against the remote, the remote branch it read is the record's. The review plan belongs to the Task's own branch and pull request, so the plan is now settled from that read only when the record names the same branch and pull request as the confirmed request. For any other record the reconciled record is written as before and the plan is left alone. --- src-tauri/src/commands/pr/publication.rs | 53 +++++++++++++++++++++++- 1 file changed, 51 insertions(+), 2 deletions(-) diff --git a/src-tauri/src/commands/pr/publication.rs b/src-tauri/src/commands/pr/publication.rs index 960f4ca..a5700f3 100644 --- a/src-tauri/src/commands/pr/publication.rs +++ b/src-tauri/src/commands/pr/publication.rs @@ -384,8 +384,13 @@ async fn publish( }; let updated = PrPublication { state: state_now, updated_at: chrono::Utc::now(), ..record.clone() }; // The remote was just read: when it holds the record's commit, it - // also settles the review fixes it carries. - let delivered = if updated.state == (PublicationState::Published {}) { + // 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() @@ -1399,6 +1404,50 @@ mod orchestration { 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 _ = publish(&fx.state, fx.task_id, BRANCH, &c1, PR).await; + + 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; From 1baee6beb79626b1595d3e7f6165dbe1ccebb0cd Mon Sep 17 00:00:00 2001 From: Rui Andrada <27135+shingonoide@users.noreply.github.com> Date: Sun, 1 Nov 2026 18:38:13 -0300 Subject: [PATCH 3/4] fix(pr): answer a publication request only from a record for the same branch A published record answered a repeated request, and a settled earlier record answered a request that had reconciled it, when only the commit and the pull request matched. The same commit and pull request published from another branch was then reported as the branch the person confirmed, and the panel accepted it. Replay and the post-reconcile answer check now compare the branch as well. A record for another branch is still settled against its own remote, but it neither answers nor publishes the current request, which is refused so the person can confirm it again. The panel applies the same identity to the answer it receives. --- src-tauri/src/commands/pr/publication.rs | 104 +++++++++++++++++++++-- src-tauri/src/domain/publication.rs | 24 ++++-- src/components/pr_publication.rs | 15 ++-- src/components/pr_publication_state.rs | 27 ++++-- 4 files changed, 143 insertions(+), 27 deletions(-) diff --git a/src-tauri/src/commands/pr/publication.rs b/src-tauri/src/commands/pr/publication.rs index a5700f3..ea27e7e 100644 --- a/src-tauri/src/commands/pr/publication.rs +++ b/src-tauri/src/commands/pr/publication.rs @@ -28,8 +28,11 @@ //! 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}; @@ -364,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 => { @@ -398,14 +401,16 @@ async fn publish( 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), )); @@ -1060,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; @@ -1419,8 +1506,9 @@ mod orchestration { fx.leave_record(PrPublication { branch: other.to_string(), ..pending_record(&c1, &fx.base) }).await; test_seam::fix_observation(true, BRANCH, true); - let _ = publish(&fx.state, fx.task_id, BRANCH, &c1, PR).await; + 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"); diff --git a/src-tauri/src/domain/publication.rs b/src-tauri/src/domain/publication.rs index 472ba89..91ed8c8 100644 --- a/src-tauri/src/domain/publication.rs +++ b/src-tauri/src/domain/publication.rs @@ -234,13 +234,15 @@ pub enum Replay { } /// 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/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); From ba5526ad5e3c63e1d30ffb6172d0221b56974586 Mon Sep 17 00:00:00 2001 From: Rui Andrada <27135+shingonoide@users.noreply.github.com> Date: Sun, 1 Nov 2026 19:00:41 -0300 Subject: [PATCH 4/4] docs(pr): name the branch in the publication replay comments The replay and publish documentation still described a record as matching on the commit and pull request alone. Say that the branch is part of the identity too. --- src-tauri/src/commands/pr/publication.rs | 8 ++++---- src-tauri/src/domain/publication.rs | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src-tauri/src/commands/pr/publication.rs b/src-tauri/src/commands/pr/publication.rs index ea27e7e..4a38f34 100644 --- a/src-tauri/src/commands/pr/publication.rs +++ b/src-tauri/src/commands/pr/publication.rs @@ -72,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. diff --git a/src-tauri/src/domain/publication.rs b/src-tauri/src/domain/publication.rs index 91ed8c8..0a8d2ff 100644 --- a/src-tauri/src/domain/publication.rs +++ b/src-tauri/src/domain/publication.rs @@ -226,8 +226,8 @@ 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,