diff --git a/crates/slashit-attention/src/lib.rs b/crates/slashit-attention/src/lib.rs index 62d9713d..e5d765ec 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 creation 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, + ) } } @@ -160,14 +173,26 @@ 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 +/// 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. 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, integration_pending: bool, + pr_create_unconfirmed: bool, ) -> Option { match stage { Stage::Failed => Some(AttentionReason::Failed), @@ -176,6 +201,9 @@ pub fn needs_you( } 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::Approved { pr_failed: true, pr_linked: false, @@ -201,7 +229,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 +237,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 +258,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 +271,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 +291,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 +311,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 +367,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 +386,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 +399,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 +416,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 +428,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 +447,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 +472,127 @@ 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() { + // An undecided arrival asks for its review first; see the order test. + for stage in [ + 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) + ); + } + + /// 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, 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)), + (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); + 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..d69d83ca 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() } @@ -3598,6 +3599,165 @@ mod tests { assert_eq!(task.activity[0].kind, ActivityKind::PrMerged { number: 7 }); } + #[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" + ); + } + + 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() { 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() {