From 49bc4dfbe4691b5959f1f1075655636868bec967 Mon Sep 17 00:00:00 2001 From: shiva Date: Tue, 25 Aug 2026 14:09:25 +0530 Subject: [PATCH] fix: verify a matched PR is this item's own before trusting it (item #63) find_existing matches open/closed/merged PRs by branch name alone, and branch names get reused across items over time. push_and_open_pr already guards against treating a stale, unrelated, already-merged match as "this item's PR already exists" (item #63). is_pr_merged, relabel_pr_completed, and pr_ci_status had the identical hole: each trusted any find_existing match without confirming it via marks_item, so an unrelated PR sharing the branch name could fool check_merge into promoting the wrong item, relabel the wrong PR, or report the wrong item's CI status. Adds marks_item unit tests, including the #6 vs #63 digit-boundary collision in both directions. Agentflare-Agent: claude-code Agentflare-Branch: task-70-review-fix-tmp --- src/github/models.rs | 2 ++ src/github/pulls.rs | 63 ++++++++++++++++++++++++++++++++++++++++++-- src/worktree.rs | 54 ++++++++++++++++++++++++++++++++----- 3 files changed, 111 insertions(+), 8 deletions(-) diff --git a/src/github/models.rs b/src/github/models.rs index 324b5a8c..eb474b43 100644 --- a/src/github/models.rs +++ b/src/github/models.rs @@ -20,6 +20,8 @@ pub struct PullRequest { pub state: String, pub title: String, #[serde(default)] + pub body: Option, + #[serde(default)] pub draft: bool, // Present (non-null) on both the list and single-PR endpoints, unlike // `merged: bool` which the GitHub API only returns from the single-PR diff --git a/src/github/pulls.rs b/src/github/pulls.rs index 55198d0f..515970d4 100644 --- a/src/github/pulls.rs +++ b/src/github/pulls.rs @@ -60,6 +60,25 @@ pub fn find_existing( .find(|pr| pr.head.as_ref().is_some_and(|h| h.git_ref == branch))) } +/// The `for item # ` marker `pr_footer` stamps onto every PR +/// agentflare opens, shared by `find_by_item_marker`'s search query and +/// `marks_item`'s body check below. +fn item_marker(sequence_id: i64) -> String { + format!("for item #{sequence_id} ") +} + +/// True if `body` carries `item_marker(sequence_id)` -- i.e. this PR really +/// is `sequence_id`'s own, as opposed to an unrelated PR that only happens +/// to share the same branch name. Branch names get reused across items over +/// time, so a closed/merged `find_existing` match needs this confirmation +/// before a caller treats it as "this item's PR already exists" (item #63: +/// a stale, unrelated, already-merged PR from a prior item was returned as +/// the current item's `pr_url`, which made `in_review` true and skipped the +/// `nothing_was_ever_committed` safety net for real, uncommitted work). +pub fn marks_item(body: Option<&str>, sequence_id: i64) -> bool { + body.is_some_and(|b| b.contains(&item_marker(sequence_id))) +} + /// Finds every PR (open, merged, or closed) whose body carries the /// `for item #` marker `pr_footer` stamps onto every PR /// agentflare opens (see `push_and_open_pr`) -- the pre-dispatch @@ -76,8 +95,10 @@ pub fn find_by_item_marker( sequence_id: i64, ) -> Result, GitHubError> { let query = format!( - "repo:{}/{} type:pr \"for item #{sequence_id} \" in:body", - repo.owner, repo.repo + "repo:{}/{} type:pr \"{}\" in:body", + repo.owner, + repo.repo, + item_marker(sequence_id) ); let path = format!("/search/issues?q={}", crate::github::encode_query(&query)); let items = client.get_paginated(&path, search_items)?; @@ -293,6 +314,44 @@ mod tests { ); } + #[test] + fn marks_item_true_when_body_carries_the_marker() { + assert!(marks_item( + Some("---\n_Opened by `claude-code` on **box** for item #63 via agentflare._"), + 63 + )); + } + + #[test] + fn marks_item_false_when_body_is_none() { + assert!(!marks_item(None, 63)); + } + + #[test] + fn marks_item_false_when_body_has_no_marker_at_all() { + assert!(!marks_item(Some("just a regular PR description"), 63)); + } + + #[test] + fn marks_item_does_not_let_a_shorter_id_match_a_longer_ones_marker() { + // A PR marked "for item #63 " must not also count as evidence for + // item #6 -- naive substring matching without the marker's own + // digit-boundary delimiter would let "for item #6" match inside + // "for item #63 ". + assert!(!marks_item( + Some("---\n_Opened by `claude-code` on **box** for item #63 via agentflare._"), + 6 + )); + } + + #[test] + fn marks_item_does_not_let_a_longer_id_match_a_shorter_ones_marker() { + assert!(!marks_item( + Some("---\n_Opened by `claude-code` on **box** for item #6 via agentflare._"), + 63 + )); + } + #[test] fn find_by_item_marker_searches_and_fetches_each_matching_pr() { let server = MockServer::start(vec![ diff --git a/src/worktree.rs b/src/worktree.rs index 78687eb9..6994f01f 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -69,6 +69,12 @@ pub fn commit_uncommitted( /// resolvable remote, or a lookup failure all just report "not merged yet" /// rather than erroring, since the caller's fallback is simply to check /// again later. +/// +/// `find_existing` matches on branch name alone, and branch names get +/// reused across items over time, so a match is only trusted as this +/// item's own PR when `marks_item` confirms it -- otherwise an unrelated, +/// already-merged PR from a past item would fool `check_merge` into +/// promoting this item off someone else's merge (item #63). pub fn is_pr_merged(item: &agentflare_backend::item::Item, repo_root: &Path) -> bool { let branch = flare_git_core::worktree::resolve_item_task_branch(item, repo_root); let Some(repo) = RepoId::resolve_from_remote(repo_root) else { @@ -79,7 +85,10 @@ pub fn is_pr_merged(item: &agentflare_backend::item::Item, repo_root: &Path) -> Err(_) => return false, }; match crate::github::pulls::find_existing(&client, &repo, &branch) { - Ok(Some(pr)) => pr.merged_at.is_some(), + Ok(Some(pr)) => { + pr.merged_at.is_some() + && crate::github::pulls::marks_item(pr.body.as_deref(), item.sequence_id) + } Ok(None) => false, Err(e) => { eprintln!( @@ -97,6 +106,11 @@ pub fn is_pr_merged(item: &agentflare_backend::item::Item, repo_root: &Path) -> /// -- it identifies who did the work, not what stage it's in. Best-effort /// like the rest of this module: a label failure here must never undo (or /// even appear to block) a DB promotion that has already happened. +/// +/// Same branch-reuse hazard as `is_pr_merged`: a `find_existing` match is +/// only relabeled once `marks_item` confirms it's this item's own PR, so an +/// unrelated PR that happens to share the branch name never gets its +/// labels touched on this item's behalf (item #63). pub fn relabel_pr_completed(item: &agentflare_backend::item::Item, repo_root: &Path) { let branch = flare_git_core::worktree::resolve_item_task_branch(item, repo_root); let Some(repo) = RepoId::resolve_from_remote(repo_root) else { @@ -107,8 +121,10 @@ pub fn relabel_pr_completed(item: &agentflare_backend::item::Item, repo_root: &P Err(_) => return, }; let pr = match crate::github::pulls::find_existing(&client, &repo, &branch) { - Ok(Some(pr)) => pr, - Ok(None) => return, + Ok(Some(pr)) if crate::github::pulls::marks_item(pr.body.as_deref(), item.sequence_id) => { + pr + } + Ok(Some(_)) | Ok(None) => return, Err(e) => { eprintln!( "worktree: could not look up PR to relabel for item {}: {e}", @@ -155,6 +171,11 @@ pub enum PrCiStatus { /// Same "total>0 && not pending" gate `cli::git::wait_for_checks` polls on, /// applied once instead of in a loop -- the sweep itself provides the retry /// cadence across ticks. +/// +/// Same branch-reuse hazard as `is_pr_merged`: a `find_existing` match is +/// only trusted once `marks_item` confirms it's this item's own PR, so an +/// unrelated PR sharing the branch name can't report its CI status as this +/// item's (item #63). pub fn pr_ci_status(item: &agentflare_backend::item::Item, repo_root: &Path) -> PrCiStatus { let branch = flare_git_core::worktree::resolve_item_task_branch(item, repo_root); let Some(repo) = RepoId::resolve_from_remote(repo_root) else { @@ -165,8 +186,10 @@ pub fn pr_ci_status(item: &agentflare_backend::item::Item, repo_root: &Path) -> Err(_) => return PrCiStatus::Unknown, }; let pr = match crate::github::pulls::find_existing(&client, &repo, &branch) { - Ok(Some(pr)) => pr, - Ok(None) => return PrCiStatus::Unknown, + Ok(Some(pr)) if crate::github::pulls::marks_item(pr.body.as_deref(), item.sequence_id) => { + pr + } + Ok(Some(_)) | Ok(None) => return PrCiStatus::Unknown, Err(e) => { eprintln!( "worktree: could not check PR status for item {}: {e}", @@ -370,13 +393,32 @@ pub fn push_and_open_pr( // soft-failed the same way the rest of this function is: log and fall // through to `create`, since a rare duplicate is a far smaller harm // than silently never opening a PR on a lookup hiccup. + // + // A closed/merged match is only trusted as *this item's own* prior PR + // when its body carries this item's marker -- branch names get reused + // across items over time, and `find_existing` matches on branch name + // alone, so an unrelated, already-merged PR from a past item can share + // this branch's name (item #63: that stale match got returned as + // `pr_url`, which made `in_review` true and skipped the + // `nothing_was_ever_committed` safety net for real, uncommitted work). + // An open match is always trusted regardless of its body, since GitHub + // itself would reject creating a genuine duplicate against it anyway. match crate::github::pulls::find_existing(&client, &repo, &branch) { - Ok(Some(existing)) => { + Ok(Some(existing)) + if existing.state == "open" + || crate::github::pulls::marks_item(existing.body.as_deref(), item.sequence_id) => + { if let Some(p) = progress { p.send(1.0, Some(1.0), Some("PR already exists".into())); } return Some(existing.html_url); } + Ok(Some(existing)) => { + eprintln!( + "worktree: found a {} PR #{} on branch {branch} but it isn't item {}'s own PR -- opening a new one", + existing.state, existing.number, item.id + ); + } Ok(None) => {} Err(e) => { eprintln!(