From 37ed6103c9a192f0d622186876717595d6463a61 Mon Sep 17 00:00:00 2001 From: Rui Andrada <27135+shingonoide@users.noreply.github.com> Date: Mon, 2 Nov 2026 02:34:03 -0300 Subject: [PATCH 1/4] task: Task B, Needs You, unknown PR creation outcome --- crates/slashit-attention/src/lib.rs | 164 ++++++++++++++++++++++++---- src-tauri/src/domain/task.rs | 65 +++++++++++ src/components/attention.rs | 31 +++++- src/models/task.rs | 22 ++++ 4 files changed, 262 insertions(+), 20 deletions(-) diff --git a/crates/slashit-attention/src/lib.rs b/crates/slashit-attention/src/lib.rs index 62d9713d..21b5da88 100644 --- a/crates/slashit-attention/src/lib.rs +++ b/crates/slashit-attention/src/lib.rs @@ -33,6 +33,10 @@ pub enum AttentionReason { /// finished: its plan is on disk and only a person can resume or discard /// it. Nothing continues it after a restart. IntegrationUnfinished, + /// A pull request was asked for and SlashIt never learned whether GitHub + /// opened one. It may exist without the task recording it, so only a + /// person can settle it, and nothing retries until they do. + PrCreateUnconfirmed, } impl AttentionReason { @@ -44,6 +48,7 @@ impl AttentionReason { Self::Review => "Review", Self::PrNotCreated => "PR not created", Self::IntegrationUnfinished => "Integration unfinished", + Self::PrCreateUnconfirmed => "PR creation outcome unknown", } } @@ -55,6 +60,7 @@ impl AttentionReason { Self::Review => "review", Self::PrNotCreated => "pr_not_created", Self::IntegrationUnfinished => "integration_unfinished", + Self::PrCreateUnconfirmed => "pr_create_unconfirmed", } } } @@ -91,6 +97,8 @@ pub struct Facts { pub delivery_in_flight: bool, /// The record holds a local integration that is still `Pending`. pub integration_pending: bool, + /// The record holds a pull request request whose outcome is unconfirmed. + pub pr_create_unconfirmed: bool, } impl Facts { @@ -109,7 +117,12 @@ impl Facts { Status::Done => Stage::Done, Status::Other => Stage::Elsewhere, }; - needs_you(stage, self.delivery_in_flight, self.integration_pending) + needs_you( + stage, + self.delivery_in_flight, + self.integration_pending, + self.pr_create_unconfirmed, + ) } } @@ -164,10 +177,16 @@ pub enum Review { /// integration outranks every Human Review reason: the target branch may /// already have moved. It is not hidden by `delivery_in_flight`, which /// describes a pull request, never an integration. +/// +/// `pr_create_unconfirmed` is whether a request to open a pull request has an +/// unconfirmed outcome on the record. It asks for the user in any status, but +/// not while a pull request is being opened: that attempt is the one SlashIt +/// is waiting on, and it settles the record when it answers. pub fn needs_you( stage: Stage, delivery_in_flight: bool, integration_pending: bool, + pr_create_unconfirmed: bool, ) -> Option { match stage { Stage::Failed => Some(AttentionReason::Failed), @@ -175,6 +194,9 @@ pub fn needs_you( Some(AttentionReason::IntegrationUnfinished) } Stage::HumanReview(_) if delivery_in_flight => None, + _ if pr_create_unconfirmed && !delivery_in_flight => { + Some(AttentionReason::PrCreateUnconfirmed) + } Stage::HumanReview(Review::Undecided) => Some(AttentionReason::Review), Stage::HumanReview(Review::Approved { pr_failed: true, @@ -201,7 +223,7 @@ mod tests { #[test] fn a_failed_task_needs_the_user() { assert_eq!( - needs_you(Stage::Failed, false, false), + needs_you(Stage::Failed, false, false, false), Some(AttentionReason::Failed) ); } @@ -209,12 +231,20 @@ mod tests { #[test] fn human_review_alone_is_not_attention_only_an_undecided_one_is() { assert_eq!( - needs_you(Stage::HumanReview(Review::Undecided), false, false), + needs_you(Stage::HumanReview(Review::Undecided), false, false, false), Some(AttentionReason::Review) ); - assert_eq!(needs_you(Stage::HumanReview(APPROVED), false, false), None); assert_eq!( - needs_you(Stage::HumanReview(Review::ChangesRequested), false, false), + needs_you(Stage::HumanReview(APPROVED), false, false, false), + None + ); + assert_eq!( + needs_you( + Stage::HumanReview(Review::ChangesRequested), + false, + false, + false + ), None ); } @@ -222,7 +252,7 @@ mod tests { #[test] fn an_approval_whose_pull_request_failed_needs_the_user() { assert_eq!( - needs_you(Stage::HumanReview(PR_FAILED), false, false), + needs_you(Stage::HumanReview(PR_FAILED), false, false, false), Some(AttentionReason::PrNotCreated) ); } @@ -235,14 +265,17 @@ mod tests { pr_failed: true, pr_linked: true, }; - assert_eq!(needs_you(Stage::HumanReview(linked), false, false), None); + assert_eq!( + needs_you(Stage::HumanReview(linked), false, false, false), + None + ); } #[test] fn nothing_in_human_review_needs_the_user_while_its_pull_request_is_being_opened() { for review in [Review::Undecided, APPROVED, PR_FAILED] { assert_eq!( - needs_you(Stage::HumanReview(review), true, false), + needs_you(Stage::HumanReview(review), true, false, false), None, "{review:?}" ); @@ -252,15 +285,15 @@ mod tests { #[test] fn a_failure_is_not_hidden_by_a_delivery_flag() { assert_eq!( - needs_you(Stage::Failed, true, false), + needs_you(Stage::Failed, true, false, false), Some(AttentionReason::Failed) ); } #[test] fn every_other_status_is_left_alone() { - assert_eq!(needs_you(Stage::Elsewhere, false, false), None); - assert_eq!(needs_you(Stage::Elsewhere, true, false), None); + assert_eq!(needs_you(Stage::Elsewhere, false, false, false), None); + assert_eq!(needs_you(Stage::Elsewhere, true, false, false), None); } #[test] @@ -272,6 +305,7 @@ mod tests { pr_linked, delivery_in_flight: false, integration_pending: false, + pr_create_unconfirmed: false, }; let approved = Some(Decision::Approved); assert_eq!( @@ -327,6 +361,11 @@ mod tests { "Integration unfinished", "integration_unfinished", ), + ( + AttentionReason::PrCreateUnconfirmed, + "PR creation outcome unknown", + "pr_create_unconfirmed", + ), ] { assert_eq!(reason.label(), label); assert_eq!(reason.as_str(), name); @@ -341,11 +380,11 @@ mod tests { fn an_unfinished_integration_needs_the_user_in_human_review_and_when_done() { for stage in [Stage::HumanReview(APPROVED), Stage::Done] { assert_eq!( - needs_you(stage, false, true), + needs_you(stage, false, true, false), Some(AttentionReason::IntegrationUnfinished), "{stage:?}" ); - assert_eq!(needs_you(stage, false, false), None, "{stage:?}"); + assert_eq!(needs_you(stage, false, false, false), None, "{stage:?}"); } } @@ -354,11 +393,16 @@ mod tests { #[test] fn an_undecided_arrival_asks_for_its_review_even_with_an_old_pending_record() { assert_eq!( - needs_you(Stage::HumanReview(Review::Undecided), false, true), + needs_you(Stage::HumanReview(Review::Undecided), false, true, false), Some(AttentionReason::Review) ); assert_eq!( - needs_you(Stage::HumanReview(Review::ChangesRequested), false, true), + needs_you( + Stage::HumanReview(Review::ChangesRequested), + false, + true, + false + ), None ); } @@ -366,11 +410,11 @@ mod tests { #[test] fn an_unfinished_integration_outranks_the_other_review_reasons_but_not_a_failure() { assert_eq!( - needs_you(Stage::HumanReview(PR_FAILED), false, true), + needs_you(Stage::HumanReview(PR_FAILED), false, true, false), Some(AttentionReason::IntegrationUnfinished) ); assert_eq!( - needs_you(Stage::Failed, false, true), + needs_you(Stage::Failed, false, true, false), Some(AttentionReason::Failed) ); } @@ -378,14 +422,14 @@ mod tests { #[test] fn an_opening_pull_request_does_not_hide_an_unfinished_integration() { assert_eq!( - needs_you(Stage::HumanReview(APPROVED), true, true), + needs_you(Stage::HumanReview(APPROVED), true, true, false), Some(AttentionReason::IntegrationUnfinished) ); } #[test] fn an_integration_record_waits_silently_in_statuses_that_offer_no_way_to_finish_it() { - assert_eq!(needs_you(Stage::Elsewhere, false, true), None); + assert_eq!(needs_you(Stage::Elsewhere, false, true, false), None); } #[test] @@ -397,6 +441,7 @@ mod tests { pr_linked: false, delivery_in_flight: false, integration_pending: true, + pr_create_unconfirmed: false, }; assert_eq!( facts(Status::HumanReview).needs_you(), @@ -421,4 +466,85 @@ mod tests { "an Integrated or absent record asks nothing" ); } + + /// An unresolved record asks for the user in every status it can be + /// read in, once no pull request is being opened. + #[test] + fn an_unconfirmed_pr_create_needs_the_user_when_idle_in_any_status() { + for stage in [ + Stage::HumanReview(Review::Undecided), + Stage::HumanReview(APPROVED), + Stage::HumanReview(PR_FAILED), + Stage::HumanReview(Review::ChangesRequested), + Stage::Done, + Stage::Elsewhere, + ] { + assert_eq!( + needs_you(stage, false, false, true), + Some(AttentionReason::PrCreateUnconfirmed), + "{stage:?}" + ); + } + } + + #[test] + fn an_active_pr_operation_suppresses_an_unconfirmed_pr_create() { + for stage in [ + Stage::HumanReview(Review::Undecided), + Stage::HumanReview(APPROVED), + Stage::HumanReview(PR_FAILED), + Stage::Done, + Stage::Elsewhere, + ] { + assert_eq!(needs_you(stage, true, false, true), None, "{stage:?}"); + } + } + + #[test] + fn an_unconfirmed_pr_create_does_not_outrank_a_failure_or_an_unfinished_integration() { + assert_eq!( + needs_you(Stage::Failed, false, false, true), + Some(AttentionReason::Failed) + ); + assert_eq!( + needs_you(Stage::HumanReview(APPROVED), false, true, true), + Some(AttentionReason::IntegrationUnfinished) + ); + assert_eq!( + needs_you(Stage::Done, false, true, true), + Some(AttentionReason::IntegrationUnfinished) + ); + } + + #[test] + fn a_resolved_record_leaves_the_rest_of_the_rule_unchanged() { + assert_eq!(needs_you(Stage::Elsewhere, false, false, false), None); + assert_eq!( + needs_you(Stage::HumanReview(Review::Undecided), false, false, false), + Some(AttentionReason::Review) + ); + assert_eq!( + needs_you(Stage::HumanReview(PR_FAILED), false, false, false), + Some(AttentionReason::PrNotCreated) + ); + } + + #[test] + fn facts_carry_an_unconfirmed_pr_create_through_the_rule() { + let facts = |delivery_in_flight, pr_create_unconfirmed| Facts { + status: Status::HumanReview, + decision: Some(Decision::Approved), + pr_error_recorded: false, + pr_linked: false, + delivery_in_flight, + integration_pending: false, + pr_create_unconfirmed, + }; + assert_eq!( + facts(false, true).needs_you(), + Some(AttentionReason::PrCreateUnconfirmed) + ); + assert_eq!(facts(true, true).needs_you(), None); + assert_eq!(facts(false, false).needs_you(), None); + } } diff --git a/src-tauri/src/domain/task.rs b/src-tauri/src/domain/task.rs index 4f46d61e..0f988988 100644 --- a/src-tauri/src/domain/task.rs +++ b/src-tauri/src/domain/task.rs @@ -1296,6 +1296,7 @@ impl Task { .local_integration .as_ref() .is_some_and(|r| r.is_pending()), + pr_create_unconfirmed: self.pr_create_unconfirmed.is_some(), } .needs_you() } @@ -3599,6 +3600,70 @@ mod tests { } /// Every origin survives the task file (TOML) and IPC (JSON) unchanged. + #[test] + fn pr_create_unconfirmed_round_trips_through_toml_and_json() { + #[derive(Serialize, Deserialize)] + struct File { + tasks: Vec, + } + let record = UnconfirmedPrCreate { + branch: "task-branch".to_string(), + commit: "a".repeat(40), + started_at: chrono::Utc::now(), + }; + let mut task = startable_task(); + task.pr_create_unconfirmed = Some(record.clone()); + + let toml_text = toml::to_string_pretty(&File { + tasks: vec![task.clone()], + }) + .unwrap(); + let loaded: File = toml::from_str(&toml_text).expect(&toml_text); + assert_eq!( + loaded.tasks[0].pr_create_unconfirmed.as_ref(), + Some(&record), + "{toml_text}" + ); + + let json = serde_json::to_string(&task).unwrap(); + let loaded: Task = serde_json::from_str(&json).unwrap(); + assert_eq!( + loaded.pr_create_unconfirmed.as_ref(), + Some(&record), + "{json}" + ); + } + + #[test] + fn an_unconfirmed_pr_create_needs_the_user_until_it_is_resolved() { + let mut task = arrived_in_review(); + task.human_review + .push(HumanReviewDecision::Approved, None, chrono::Utc::now()); + assert_eq!(task.needs_you(false), None); + + task.pr_create_unconfirmed = Some(UnconfirmedPrCreate { + branch: "task-branch".to_string(), + commit: "a".repeat(40), + started_at: chrono::Utc::now(), + }); + assert_eq!( + task.needs_you(false), + Some(AttentionReason::PrCreateUnconfirmed) + ); + assert_eq!( + task.needs_you(true), + None, + "a pull request being opened right now is the attempt SlashIt is waiting on" + ); + + task.pr_create_unconfirmed = None; + assert_eq!( + task.needs_you(false), + None, + "a resolved record asks nothing" + ); + } + #[test] fn branch_origin_round_trips_through_toml_and_json() { #[derive(Serialize, Deserialize)] diff --git a/src/components/attention.rs b/src/components/attention.rs index 278d39fd..94cde843 100644 --- a/src/components/attention.rs +++ b/src/components/attention.rs @@ -157,7 +157,8 @@ pub fn next_after(order: &[Uuid], current: Option) -> Option { /// Classes for a reason's chip: red for a failure, violet for a review, /// amber for a pull request that was not created, orange for an integration -/// that was started and not finished. +/// that was started and not finished, fuchsia for a pull request whose +/// outcome is unknown. pub fn chip_classes(reason: AttentionReason) -> &'static str { match reason { AttentionReason::Recovery => "bg-red-500/15 text-red-200 border-red-500/40", @@ -167,6 +168,9 @@ pub fn chip_classes(reason: AttentionReason) -> &'static str { AttentionReason::IntegrationUnfinished => { "bg-orange-500/15 text-orange-200 border-orange-400/40" } + AttentionReason::PrCreateUnconfirmed => { + "bg-fuchsia-500/15 text-fuchsia-200 border-fuchsia-400/40" + } } } @@ -300,11 +304,36 @@ mod tests { assert_eq!(next_after(&[], None), None); } + #[test] + fn an_unconfirmed_pr_create_is_listed_while_idle_and_until_it_is_resolved() { + let mut waiting = task("backlog", 0, 6); + waiting.pr_create_unconfirmed = serde_json::from_value(serde_json::json!({ + "branch": "task-branch", + "commit": "a".repeat(40), + "started_at": "2026-01-01T00:00:00Z", + })) + .unwrap(); + let id = Uuid::from_u128(6); + let reason = AttentionReason::PrCreateUnconfirmed; + assert_eq!( + attention_order(std::slice::from_ref(&waiting), rank, |_| false), + vec![(id, reason)] + ); + assert!( + attention_order(std::slice::from_ref(&waiting), rank, |_| true).is_empty(), + "a pull request being opened right now is what SlashIt is waiting on" + ); + + waiting.pr_create_unconfirmed = None; + assert!(attention_order(&[waiting], rank, |_| false).is_empty()); + } + #[test] fn each_reason_has_its_own_colour() { assert!(chip_classes(AttentionReason::Failed).contains("red")); assert!(chip_classes(AttentionReason::Review).contains("violet")); assert!(chip_classes(AttentionReason::PrNotCreated).contains("amber")); assert!(chip_classes(AttentionReason::IntegrationUnfinished).contains("orange")); + assert!(chip_classes(AttentionReason::PrCreateUnconfirmed).contains("fuchsia")); } } diff --git a/src/models/task.rs b/src/models/task.rs index a7ce3a1e..edcac4d1 100644 --- a/src/models/task.rs +++ b/src/models/task.rs @@ -254,6 +254,7 @@ impl Task { .local_integration .as_ref() .is_some_and(|r| r.state == crate::models::integration::IntegrationState::Pending), + pr_create_unconfirmed: self.pr_create_unconfirmed.is_some(), } .needs_you() } @@ -1382,6 +1383,27 @@ mod tests { task } + /// The same answer as the backend's `an_unconfirmed_pr_create_needs_the_user_until_it_is_resolved`. + #[test] + fn an_unconfirmed_pr_create_needs_the_user_until_it_is_resolved() { + let approved = decided("approved", None); + let mut task = task_json("human_review", approved, None); + assert_eq!(task.needs_you(false), None); + task.pr_create_unconfirmed = serde_json::from_value(serde_json::json!({ + "branch": "task-branch", + "commit": "a".repeat(40), + "started_at": "2026-01-01T00:00:00Z", + })) + .unwrap(); + assert_eq!( + task.needs_you(false), + Some(AttentionReason::PrCreateUnconfirmed) + ); + assert_eq!(task.needs_you(true), None); + task.pr_create_unconfirmed = None; + assert_eq!(task.needs_you(false), None); + } + /// The same table as the backend's `a_pending_integration_needs_the_user_...`. #[test] fn a_pending_integration_needs_the_user_exactly_as_the_backend_says() { From 3d27d1db1485a2c901962d762cb1d016006df9c2 Mon Sep 17 00:00:00 2001 From: Rui Andrada <27135+shingonoide@users.noreply.github.com> Date: Mon, 2 Nov 2026 02:35:54 -0300 Subject: [PATCH 2/4] task: Task B, Needs You, unknown PR creation outcome (with review fixes) --- crates/slashit-attention/src/lib.rs | 8 ++++++-- src-tauri/src/domain/task.rs | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/crates/slashit-attention/src/lib.rs b/crates/slashit-attention/src/lib.rs index 21b5da88..38c505f2 100644 --- a/crates/slashit-attention/src/lib.rs +++ b/crates/slashit-attention/src/lib.rs @@ -97,7 +97,7 @@ pub struct Facts { pub delivery_in_flight: bool, /// The record holds a local integration that is still `Pending`. pub integration_pending: bool, - /// The record holds a pull request request whose outcome is unconfirmed. + /// The record holds a pull request creation whose outcome is unconfirmed. pub pr_create_unconfirmed: bool, } @@ -181,7 +181,11 @@ pub enum Review { /// `pr_create_unconfirmed` is whether a request to open a pull request has an /// unconfirmed outcome on the record. It asks for the user in any status, but /// not while a pull request is being opened: that attempt is the one SlashIt -/// is waiting on, and it settles the record when it answers. +/// is waiting on, and it settles the record when it answers. Among the Human +/// Review reasons it takes precedence over `Review` and `PrNotCreated`: the +/// person must learn whether the pull request exists before deciding or +/// retrying, and the outcome they are shown is the unknown one, not the +/// earlier failure. It stays below a failure and an unfinished integration. pub fn needs_you( stage: Stage, delivery_in_flight: bool, diff --git a/src-tauri/src/domain/task.rs b/src-tauri/src/domain/task.rs index 0f988988..3c337a5c 100644 --- a/src-tauri/src/domain/task.rs +++ b/src-tauri/src/domain/task.rs @@ -3599,7 +3599,6 @@ mod tests { assert_eq!(task.activity[0].kind, ActivityKind::PrMerged { number: 7 }); } - /// Every origin survives the task file (TOML) and IPC (JSON) unchanged. #[test] fn pr_create_unconfirmed_round_trips_through_toml_and_json() { #[derive(Serialize, Deserialize)] @@ -3664,6 +3663,7 @@ mod tests { ); } + /// Every origin survives the task file (TOML) and IPC (JSON) unchanged. #[test] fn branch_origin_round_trips_through_toml_and_json() { #[derive(Serialize, Deserialize)] From ea6b916689989dddb44dcb50eba3ef00ce007204 Mon Sep 17 00:00:00 2001 From: Rui Andrada <27135+shingonoide@users.noreply.github.com> Date: Mon, 2 Nov 2026 03:12:33 -0300 Subject: [PATCH 3/4] Show an undecided review before an unresolved PR creation A Task in Human Review with an undecided arrival is now asked for its review even when an older pull request creation has an unresolved outcome. The unresolved outcome is then shown ahead of a confirmed pull request failure, so an attempt whose answer is unknown is never described as a confirmed failure to create the pull request. The order is stage-dependent and unchanged elsewhere: failures, pending integrations, and delivery in flight keep their existing precedence, and worker recovery is still checked before attention facts. --- crates/slashit-attention/src/lib.rs | 56 ++++++++++++++--- src-tauri/src/domain/task.rs | 95 +++++++++++++++++++++++++++++ 2 files changed, 144 insertions(+), 7 deletions(-) diff --git a/crates/slashit-attention/src/lib.rs b/crates/slashit-attention/src/lib.rs index 38c505f2..2721ae71 100644 --- a/crates/slashit-attention/src/lib.rs +++ b/crates/slashit-attention/src/lib.rs @@ -181,11 +181,12 @@ pub enum Review { /// `pr_create_unconfirmed` is whether a request to open a pull request has an /// unconfirmed outcome on the record. It asks for the user in any status, but /// not while a pull request is being opened: that attempt is the one SlashIt -/// is waiting on, and it settles the record when it answers. Among the Human -/// Review reasons it takes precedence over `Review` and `PrNotCreated`: the -/// person must learn whether the pull request exists before deciding or -/// retrying, and the outcome they are shown is the unknown one, not the -/// earlier failure. It stays below a failure and an unfinished integration. +/// is waiting on, and it settles the record when it answers. The order of the +/// reasons for a Task in Human Review is: a failure, then an unfinished +/// integration, then an undecided arrival (`Review`), then the unresolved +/// record, then a confirmed pull request failure (`PrNotCreated`). So an +/// undecided arrival is still asked for its review first, while an unresolved +/// attempt is never shown as a confirmed failure to create a pull request. pub fn needs_you( stage: Stage, delivery_in_flight: bool, @@ -198,10 +199,10 @@ pub fn needs_you( Some(AttentionReason::IntegrationUnfinished) } Stage::HumanReview(_) if delivery_in_flight => None, + Stage::HumanReview(Review::Undecided) => Some(AttentionReason::Review), _ if pr_create_unconfirmed && !delivery_in_flight => { Some(AttentionReason::PrCreateUnconfirmed) } - Stage::HumanReview(Review::Undecided) => Some(AttentionReason::Review), Stage::HumanReview(Review::Approved { pr_failed: true, pr_linked: false, @@ -475,8 +476,8 @@ mod tests { /// read in, once no pull request is being opened. #[test] fn an_unconfirmed_pr_create_needs_the_user_when_idle_in_any_status() { + // An undecided arrival asks for its review first; see the order test. for stage in [ - Stage::HumanReview(Review::Undecided), Stage::HumanReview(APPROVED), Stage::HumanReview(PR_FAILED), Stage::HumanReview(Review::ChangesRequested), @@ -520,6 +521,47 @@ mod tests { ); } + /// The effective order for a Task with an unresolved PR-creation record, + /// one row per state. This is stage-dependent, not one global ranking. + #[test] + fn the_unresolved_pr_create_order_holds_for_each_stage_and_flag() { + use AttentionReason as R; + // stage, delivery in flight, integration pending, record unresolved, expected + let rows: &[(Stage, bool, bool, bool, Option)] = &[ + // An undecided arrival is asked for its review first. + (Stage::HumanReview(Review::Undecided), false, false, true, Some(R::Review)), + (Stage::HumanReview(Review::Undecided), false, false, false, Some(R::Review)), + // An approved arrival, or one with changes requested, asks about the outcome. + (Stage::HumanReview(APPROVED), false, false, true, Some(R::PrCreateUnconfirmed)), + (Stage::HumanReview(Review::ChangesRequested), false, false, true, Some(R::PrCreateUnconfirmed)), + // An unresolved attempt is never described as a confirmed failure. + (Stage::HumanReview(PR_FAILED), false, false, true, Some(R::PrCreateUnconfirmed)), + (Stage::HumanReview(PR_FAILED), false, false, false, Some(R::PrNotCreated)), + // A pending integration and a failure outrank the unresolved record. + (Stage::HumanReview(APPROVED), false, true, true, Some(R::IntegrationUnfinished)), + (Stage::Done, false, true, true, Some(R::IntegrationUnfinished)), + (Stage::Failed, false, false, true, Some(R::Failed)), + // A pull request being opened suppresses the record and the review. + (Stage::HumanReview(Review::Undecided), true, false, true, None), + (Stage::HumanReview(APPROVED), true, false, true, None), + (Stage::HumanReview(PR_FAILED), true, false, true, None), + (Stage::Elsewhere, true, false, true, None), + // Outside Human Review, an unresolved record asks in any status. + (Stage::Elsewhere, false, false, true, Some(R::PrCreateUnconfirmed)), + (Stage::Done, false, false, true, Some(R::PrCreateUnconfirmed)), + // No outstanding record: the rest of the rule is unchanged. + (Stage::HumanReview(APPROVED), false, false, false, None), + (Stage::Elsewhere, false, false, false, None), + ]; + for &(stage, in_flight, integration, unresolved, expected) in rows { + assert_eq!( + needs_you(stage, in_flight, integration, unresolved), + expected, + "{stage:?} in_flight={in_flight} integration={integration} unresolved={unresolved}" + ); + } + } + #[test] fn a_resolved_record_leaves_the_rest_of_the_rule_unchanged() { assert_eq!(needs_you(Stage::Elsewhere, false, false, false), None); diff --git a/src-tauri/src/domain/task.rs b/src-tauri/src/domain/task.rs index 3c337a5c..d69d83ca 100644 --- a/src-tauri/src/domain/task.rs +++ b/src-tauri/src/domain/task.rs @@ -3663,6 +3663,101 @@ mod tests { ); } + fn unresolved_pr_create() -> UnconfirmedPrCreate { + UnconfirmedPrCreate { + branch: "task-branch".to_string(), + commit: "a".repeat(40), + started_at: chrono::Utc::now(), + } + } + + fn approved_in_review() -> Task { + let mut task = arrived_in_review(); + task.human_review + .push(HumanReviewDecision::Approved, None, chrono::Utc::now()); + task + } + + #[test] + fn an_undecided_arrival_is_reviewed_before_an_unresolved_pr_create() { + let mut task = arrived_in_review(); + task.pr_create_unconfirmed = Some(unresolved_pr_create()); + assert_eq!(task.needs_you(false), Some(AttentionReason::Review)); + assert_eq!( + task.needs_you(true), + None, + "a pull request being opened right now suppresses the review too" + ); + } + + #[test] + fn an_unresolved_attempt_is_not_shown_as_a_confirmed_failure() { + let mut task = approved_in_review(); + task.human_review.pr_error = Some("GitHub refused the pull request.".to_string()); + assert_eq!(task.needs_you(false), Some(AttentionReason::PrNotCreated)); + + task.pr_create_unconfirmed = Some(unresolved_pr_create()); + assert_eq!( + task.needs_you(false), + Some(AttentionReason::PrCreateUnconfirmed), + "the earlier refusal is not the outcome: the attempt's answer is unknown" + ); + } + + #[test] + fn a_verified_link_settles_the_unresolved_record() { + let mut task = approved_in_review(); + task.human_review.pr_error = Some("GitHub refused the pull request.".to_string()); + task.pr_create_unconfirmed = Some(unresolved_pr_create()); + assert_eq!( + task.needs_you(false), + Some(AttentionReason::PrCreateUnconfirmed) + ); + + // A link clears the record and leaves the pull request linked, so the + // earlier refusal no longer asks anything either. + task.pr_create_unconfirmed = None; + task.pr_url = Some("https://github.com/example/repo/pull/1".to_string()); + assert_eq!(task.needs_you(false), None); + } + + #[test] + fn a_confirmed_discard_settles_the_record_to_the_no_pull_request_state() { + let mut task = approved_in_review(); + task.human_review.pr_error = Some("GitHub refused the pull request.".to_string()); + task.pr_create_unconfirmed = Some(unresolved_pr_create()); + + // A discard clears the record and records that no pull request exists. + task.pr_create_unconfirmed = None; + task.human_review.pr_error = Some( + "You confirmed that no pull request exists. Retry to open one.".to_string(), + ); + assert_eq!(task.needs_you(false), Some(AttentionReason::PrNotCreated)); + } + + #[test] + fn worker_recovery_outranks_an_unresolved_pr_create() { + let mut task = startable_task(); + task.pr_create_unconfirmed = Some(unresolved_pr_create()); + task.worker_handoff = Some(WorkerHandoff { + review_mode: Default::default(), + task_id: task.id, + project_id: task.project_id, + repository_id: None, + run: Some(1), + branch: Some("task/example".into()), + start_head: Some("a".repeat(40)), + acceptance: None, + stage: WorkerHandoffStage::RecoveryRequired { + reason: "exact commit unknown".into(), + observed_commit: None, + }, + review_mode_unproven: false, + reviewer_model: None, + }); + assert_eq!(task.needs_you(false), Some(AttentionReason::Recovery)); + } + /// Every origin survives the task file (TOML) and IPC (JSON) unchanged. #[test] fn branch_origin_round_trips_through_toml_and_json() { From ca150d59e55f7eb8ed7d4d78b294c511e7b40df5 Mon Sep 17 00:00:00 2001 From: Rui Andrada <27135+shingonoide@users.noreply.github.com> Date: Mon, 2 Nov 2026 03:27:40 -0300 Subject: [PATCH 4/4] Pin the undecided review over an older integration record The integration precedence doc now states the exception for an undecided arrival, and the order table pins that row. --- crates/slashit-attention/src/lib.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/crates/slashit-attention/src/lib.rs b/crates/slashit-attention/src/lib.rs index 2721ae71..e5d765ec 100644 --- a/crates/slashit-attention/src/lib.rs +++ b/crates/slashit-attention/src/lib.rs @@ -173,10 +173,11 @@ pub enum Review { /// Review and with a Done task that was completed locally, so only those ask /// for it. A failed task is repaired first. In a new, undecided arrival in /// Human Review the review is what is asked; in any other status the record -/// waits for the task to come back. The unfinished -/// integration outranks every Human Review reason: the target branch may -/// already have moved. It is not hidden by `delivery_in_flight`, which -/// describes a pull request, never an integration. +/// waits for the task to come back. An unfinished integration of an approved +/// (or Done) task outranks every other reason: the target branch may already +/// have moved. It is not hidden by `delivery_in_flight`, which describes a +/// pull request, never an integration. An undecided arrival is the exception: +/// its review is asked for even when an older integration record is pending. /// /// `pr_create_unconfirmed` is whether a request to open a pull request has an /// unconfirmed outcome on the record. It asks for the user in any status, but @@ -530,6 +531,7 @@ mod tests { let rows: &[(Stage, bool, bool, bool, Option)] = &[ // An undecided arrival is asked for its review first. (Stage::HumanReview(Review::Undecided), false, false, true, Some(R::Review)), + (Stage::HumanReview(Review::Undecided), false, true, true, Some(R::Review)), (Stage::HumanReview(Review::Undecided), false, false, false, Some(R::Review)), // An approved arrival, or one with changes requested, asks about the outcome. (Stage::HumanReview(APPROVED), false, false, true, Some(R::PrCreateUnconfirmed)),