Skip to content

fix(attention): surface unconfirmed PR creation in Needs You #296

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shingonoide merged 4 commits into from
Nov 2, 2026
Merged

fix(attention): surface unconfirmed PR creation in Needs You #296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
37ed610
task: Task B, Needs You, unknown PR creation outcome
shingonoide Nov 2, 2026
3d27d1d
task: Task B, Needs You, unknown PR creation outcome (with review fixes)
shingonoide Nov 2, 2026
ea6b916
Show an undecided review before an unresolved PR creation
shingonoide Nov 2, 2026
ca150d5
Pin the undecided review over an older integration record
shingonoide Nov 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
220 changes: 197 additions & 23 deletions crates/slashit-attention/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -44,6 +48,7 @@ impl AttentionReason {
Self::Review => "Review",
Self::PrNotCreated => "PR not created",
Self::IntegrationUnfinished => "Integration unfinished",
Self::PrCreateUnconfirmed => "PR creation outcome unknown",
}
}

Expand All @@ -55,6 +60,7 @@ impl AttentionReason {
Self::Review => "review",
Self::PrNotCreated => "pr_not_created",
Self::IntegrationUnfinished => "integration_unfinished",
Self::PrCreateUnconfirmed => "pr_create_unconfirmed",
}
}
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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,
)
}
}

Expand Down Expand Up @@ -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<AttentionReason> {
match stage {
Stage::Failed => Some(AttentionReason::Failed),
Expand All @@ -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)
}
Comment on lines +204 to +206

@coderabbitai coderabbitai Bot Nov 2, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor · Functional Correctness · Quick win

Supported by static analysis

Script executed:

printf '%s\n' '--- approve implementation ---'
nl -ba src-tauri/src/commands/human_review.rs | sed -n '340,405p'
printf '%s\n' '--- delivery registration usages in human_review ---'
rg -n -F -- 'deliveries.start' src-tauri/src/commands/human_review.rs src-tauri/src/commands/attention.rs
printf '%s\n' '--- delivery API and relevant tests ---'
rg -n -F -- 'struct Deliver' src-tauri crates
rg -n -F -e 'approve_shown' -e 'create_pr: false' -e 'delivery_in_flight' src-tauri/src/commands/human_review.rs src-tauri/src/commands/pr.rs crates/slashit-attention/src/lib.rs
printf '%s\n' '--- reviewed change for approval registration ---'
git diff 14f170b13b0f864a6322576f4512dadfb18482a2 ca150d59e55f7eb8ed7d4d78b294c511e7b40df5 -- src-tauri/src/commands/human_review.rs crates/slashit-attention/src/lib.rs

Repository: BarraDev/slashit

Length of output: 27238

Register the delivery marker only for PR creation.

approve(create_pr: false) starts the delivery marker before recording the approval, then returns without starting a PR operation. A concurrent get_attention_summary call therefore hides PR creation outcome unknown while the approval-only call is in progress. This can temporarily remove an already-approved task from the backend Needs You summary.

Keep the marker before record_approval when create_pr is true. That preserves active-PR suppression and the existing deletion-safety ordering.

Suggested fix 
-    let _in_flight = state.task.deliveries.start(task_id);
+    let _in_flight = if create_pr {
+        Some(state.task.deliveries.start(task_id))
+    } else {
+        None
+    };

@shingonoide shingonoide Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed. During an approval-only call (create_pr: false), the delivery marker is registered before the approval is recorded, so get_attention_summary can omit the "PR creation outcome unknown" reason until that call returns. The effect is transient and display-only: nothing is persisted from the marker, and the marker does not replace the Project-deletion protection, which holds through the Task lease and the checkout refusal. Protection for PR creation (create_pr: true) is unchanged.

Decision: defer the correction. No change is made in this PR, so the approved head stays as reviewed. The conditional marker suggested here is a candidate fix and has not been applied or tested. This thread stays open as a documented deferred finding, and the follow-up is recorded in the engineering ledger.

@coderabbitai coderabbitai Bot Nov 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shingonoide Understood. The finding remains open as deferred. The conditional delivery marker is an untested candidate fix, not a change in this PR.

Thanks for clarifying the deletion protection. The Task lease and checkout refusal provide that protection; the delivery marker does not replace it.

Would you like a GitHub follow-up issue linked to this thread, or is the engineering ledger sufficient?

You are interacting with an AI system.

Stage::HumanReview(Review::Approved {
pr_failed: true,
pr_linked: false,
Expand All @@ -201,28 +229,36 @@ 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)
);
}

#[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
);
}

#[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)
);
}
Expand All @@ -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:?}"
);
Expand All @@ -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]
Expand All @@ -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!(
Expand Down Expand Up @@ -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);
Expand All @@ -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:?}");
}
}

Expand All @@ -354,38 +399,43 @@ 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
);
}

#[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)
);
}

#[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]
Expand All @@ -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(),
Expand All @@ -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<R>)] = &[
// 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);
}
}
Loading
Loading