Skip to content

fix(bin): accept a task's next PR after its bound PR merges in fm-pr-merge - #6053

Merged
kunchenguid merged 2 commits into
kunchenguid:mainfrom
karotkriss:fm/fm-up-5697-pr-merge-rebind
Sep 28, 2026
Merged

kunchenguid merged 2 commits into
kunchenguid:mainfrom
karotkriss:fm/fm-up-5697-pr-merge-rebind

Conversation

@karotkriss

Copy link
Copy Markdown
Contributor

Intent

Fixes #5697

When one task produces several PRs, bin/fm-pr-merge.sh merges only the first; every later merge is refused with "error: task is bound to , not ", even after already merged through fm-pr-merge.sh itself, so an operator has to re-run bin/fm-pr-check.sh by hand before each later merge.
Once the task's bound PR has merged, fm-pr-merge should accept the task's next PR; while the bound PR is still unmerged it should keep refusing.

What Changed

  • require_recorded_pr_identity in bin/fm-pr-merge.sh now accepts a task's next PR when the recorded pr= URL differs, provided that bound PR has already merged (proven by its recorded merge notification via fm_pr_poll_merge_already_notified); while the bound PR is still unmerged, a different URL is still refused. The bound URL is parsed in a subshell so FM_PR_* stays the new URL's identity for later callers.
  • Updated the header comment to document the "already-merged bound PR" exception to the pr= binding rule.
  • Reworked the test_distinct_merged_prs_keep_distinct_wakes test to rely on the confirmed-merge marker instead of hand-editing pr=/pr_head= out of the task meta before the second merge.

Risk Assessment

✅ Low: A small, well-bounded gate relaxation that follows an existing tested idiom, correctly implements both sides of the stated intent, and is covered by a behavioral test.

Testing

Drove the two end-user scenarios directly against the real bin/fm-pr-merge.sh through the test harness's run_pr_merge entrypoint: a task's second PR is accepted once the first PR's merge is notified, and a different PR is still refused while the bound one is unmerged. Both passed. Proved the regression by reverting only the fix, which made the second merge fail (refused as bound to the first URL), then restored the file and left the worktree clean.

  • Live validation: ✅ go - 3 of 3 scenarios driven live against the product
Scenario Result Live Evidence
After the bound PR merges, the task's next PR is accepted ✅ pass live test_distinct_merged_prs_keep_distinct_wakes drives real bin/fm-pr-merge.sh: merge PR/68 writes the notified marker, then merge PR/69 (pr= still bound to /68) succeeds and both leave distinct wakes
While the bound PR is unmerged, a different PR is refused ✅ pass live test_away_record_does_not_bypass_red_or_identity: pr= bound to /99 (no notified marker), merging /85 exits 1 with 'is bound to https://github.com/example/repo/pull/99'
Regression reproduces: without the fix the second merge is refused ✅ pass live Reverting the require_recorded_pr_identity change makes test_distinct_merged_prs_keep_distinct_wakes fail with 'distinct-merge-wakes: second merge failed'; restoring the fix makes it pass again
Evidence: fm-pr-merge PR-rebind scenario transcript (with fix + reverted-fix control)

Source: fm-pr-merge PR-rebind scenario transcript (with fix + reverted-fix control)

with fix: ok - distinct merged PRs for one task retain distinct captain-facing wakes / ok - the away record does not bypass red checks, and a recorded pr= must match the URL. fix reverted: not ok - distinct-merge-wakes: second merge failed (refused: task bound to pr-1, not pr-2)

## fm-pr-merge PR-rebind scenarios (real bin/fm-pr-merge.sh driven via run_pr_merge)

# with fix (target commit):
ok - distinct merged PRs for one task retain distinct captain-facing wakes
ok - the away record does not bypass red checks, and a recorded pr= must match the URL


# regression control - fix reverted, same distinct-merge scenario:
not ok - distinct-merge-wakes: second merge failed
(second merge refused with "error: task task-x1 is bound to <pr-1>, not <pr-2>")

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

✅ **Review** - passed

✅ No issues found.

✅ **Test** - passed

✅ No issues found.

  • Live validation: ✅ go - 3 of 3 scenarios driven live against the product
Scenario Result Live Evidence
After the bound PR merges, the task's next PR is accepted ✅ pass live test_distinct_merged_prs_keep_distinct_wakes drives real bin/fm-pr-merge.sh: merge PR/68 writes the notified marker, then merge PR/69 (pr= still bound to /68) succeeds and both leave distinct wakes
While the bound PR is unmerged, a different PR is refused ✅ pass live test_away_record_does_not_bypass_red_or_identity: pr= bound to /99 (no notified marker), merging /85 exits 1 with 'is bound to https://github.com/example/repo/pull/99&#39;
Regression reproduces: without the fix the second merge is refused ✅ pass live Reverting the require_recorded_pr_identity change makes test_distinct_merged_prs_keep_distinct_wakes fail with 'distinct-merge-wakes: second merge failed'; restoring the fix makes it pass again
  • bash runner sourcing tests/fm-pr-merge.test.sh definitions then invoking test_distinct_merged_prs_keep_distinct_wakes (drives real bin/fm-pr-merge.sh via run_pr_merge: merge PR/68, then merge PR/69 with pr= still bound to /68)
  • test_away_record_does_not_bypass_red_or_identity (pr= bound to /99 unmerged, merge of /85 must refuse with 'is bound to .../99')
  • Regression control: reverted the require_recorded_pr_identity fix in bin/fm-pr-merge.sh and re-ran the distinct-merge scenario, confirming the second merge is refused without the fix; restored the file afterward
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

… one merged

require_recorded_pr_identity now checks fm_pr_poll_merge_already_notified for
the recorded pr= before refusing a different URL, so a task's later PR is
accepted once its earlier PR's merge is confirmed, while it keeps refusing
while the bound PR is still unmerged.
@greptile-apps

greptile-apps Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[High risk] Changes how the merge script validates task rebinding.

The PR appears safe to merge.

Reviews (1) · Last reviewed commit: "no-mistakes(document): docs(fm-pr-merge)..."

@kunchenguid
kunchenguid merged commit eb219c8 into kunchenguid:main Sep 28, 2026
20 checks passed
@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate: this is merged. Thank you @karotkriss — really appreciate you taking the time on this.

knowttl pushed a commit to knowttl/firstmate that referenced this pull request Sep 29, 2026
…merge (kunchenguid#6053)

* fix(bin): accept a task's next PR once fm-pr-merge confirms the bound one merged

require_recorded_pr_identity now checks fm_pr_poll_merge_already_notified for
the recorded pr= before refusing a different URL, so a task's later PR is
accepted once its earlier PR's merge is confirmed, while it keeps refusing
while the bound PR is still unmerged.

* no-mistakes(document): docs(fm-pr-merge): note next-PR accepted after bound PR merges
RooseveltAdvisors pushed a commit to RooseveltAdvisors/firstmate that referenced this pull request Sep 29, 2026
…merge (kunchenguid#6053)

* fix(bin): accept a task's next PR once fm-pr-merge confirms the bound one merged

require_recorded_pr_identity now checks fm_pr_poll_merge_already_notified for
the recorded pr= before refusing a different URL, so a task's later PR is
accepted once its earlier PR's merge is confirmed, while it keeps refusing
while the bound PR is still unmerged.

* no-mistakes(document): docs(fm-pr-merge): note next-PR accepted after bound PR merges
andrewesweet pushed a commit to andrewesweet/firstmate that referenced this pull request Sep 30, 2026
…merge (kunchenguid#6053)

* fix(bin): accept a task's next PR once fm-pr-merge confirms the bound one merged

require_recorded_pr_identity now checks fm_pr_poll_merge_already_notified for
the recorded pr= before refusing a different URL, so a task's later PR is
accepted once its earlier PR's merge is confirmed, while it keeps refusing
while the bound PR is still unmerged.

* no-mistakes(document): docs(fm-pr-merge): note next-PR accepted after bound PR merges
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fm-pr-merge refuses a task's second PR even after the first one merged

2 participants