Abandoned closed-unmerged PR reverts a Done target completed by a different merged PR to Review #96

New issue

Open

opened 2026-10-15 22:30:50 +08:00 by Breadway · 0 comments

Breadway commented

2026-10-15 22:30:50 +08:00

Owner

Found during independent review of PR89 (BB-99) at 955fb2887a. Confidence: medium (reproduced once by the reviewer with a throwaway test; the reproduction is written out below so it can be turned into a maintained fixture).

Behavior: sync_pr_closes treats every closed-unmerged PR that references a target as rejecting the target. If the target is Done it is moved to Review (and reopened on Forgejo), with no check on whether a different, merged PR is what completed it.

Reproduction (crates/breadboard-server/tests/sync.rs helpers pr_issue, import_target_issue, ensure_repo_and_project):

Fresh project TST, import non-PR issue as TST-1.
apply_issue a merged PR A (closed=true, merged=true, body "Closes TST-1"). TST-1 becomes Done.
apply_issue a closed-unmerged PR B (closed=true, merged=false, body "Closes TST-1", later updated_at, source ForgejoReconcile).
Observed: TST-1 is Review. Expected by this report: stays Done, because accepted integrated work (PR A) completed it.

Related: any later update to an abandoned PR that triggers sync_pr_closes (label or title change) would re-revert a target that a human moved back to Done.

Why it is not part of PR89: the revert-to-review rule is the documented, intended conservative behavior of BB-99 (Documentation.md PR row), and the reviewer judged it non-blocking. Fixing it needs provenance (which PR completed the target) or an explicit policy decision, so it should be decided separately rather than widened into PR89.

Proposed direction: record the completing merged PR (or check for any linked closed-by PR with merged=true) and skip the revert when one exists.

Found during independent review of PR89 (BB-99) at 955fb2887adb48f91220347ae252e294984a7f24. Confidence: medium (reproduced once by the reviewer with a throwaway test; the reproduction is written out below so it can be turned into a maintained fixture). Behavior: sync_pr_closes treats every closed-unmerged PR that references a target as rejecting the target. If the target is Done it is moved to Review (and reopened on Forgejo), with no check on whether a different, merged PR is what completed it. Reproduction (crates/breadboard-server/tests/sync.rs helpers pr_issue, import_target_issue, ensure_repo_and_project): 1. Fresh project TST, import non-PR issue as TST-1. 2. apply_issue a merged PR A (closed=true, merged=true, body "Closes TST-1"). TST-1 becomes Done. 3. apply_issue a closed-unmerged PR B (closed=true, merged=false, body "Closes TST-1", later updated_at, source ForgejoReconcile). 4. Observed: TST-1 is Review. Expected by this report: stays Done, because accepted integrated work (PR A) completed it. Related: any later update to an abandoned PR that triggers sync_pr_closes (label or title change) would re-revert a target that a human moved back to Done. Why it is not part of PR89: the revert-to-review rule is the documented, intended conservative behavior of BB-99 (Documentation.md PR row), and the reviewer judged it non-blocking. Fixing it needs provenance (which PR completed the target) or an explicit policy decision, so it should be decided separately rather than widened into PR89. Proposed direction: record the completing merged PR (or check for any linked closed-by PR with merged=true) and skip the revert when one exists.