Skip to content

fix(bin): merge a pushed stacked PR while the copy sits on a later unpushed branch - #6497

Open
anconina wants to merge 20 commits into
kunchenguid:mainfrom
anconina:fm/fm-merge-stacked-head
Open

anconina wants to merge 20 commits into
kunchenguid:mainfrom
anconina:fm/fm-merge-stacked-head

Conversation

@anconina

@anconina anconina commented Oct 3, 2026 •

Copy link
Copy Markdown

I kept hitting this while building stacked PRs in one task copy. Part 1 was pushed and green, then I moved on to part 2 on a new local branch without pushing it. Merging the part-1 PR refused with "named head is unreachable outside the worker copy" because the gate was checking my current HEAD. I had to push the unfinished next part to unblock the earlier merge, about eight times in one day.

To reproduce, push branch A and open a PR, wait for its checks to pass, then create branch B and commit without pushing. Run bin/fm-pr-merge.sh for A. The old gate refuses because B exists only in the copy.

For GitHub and GitLab direct-PR tasks, the merge now checks the PR's own head branch against the head just verified on the forge. Its local tip must equal that head or be an ancestor of it. A missing head is fetched by SHA without switching branches or moving refs. Later work on another branch no longer blocks the merge; unpushed commits on the PR's own branch still do, even if another remote branch contains them. A missing local PR branch falls back to the existing HEAD gate. GitLab fork MRs only use a local branch when its upstream remote matches the source fork; an unreadable source project refuses the merge.

In no-mistakes mode, GitHub and GitLab merges bind to the pipeline-verified forge head; a later commit in the copy is left out of the merge and stays protected there. The pipeline is the publisher in that mode, so the copy can have an older branch after pipeline validation. I added behavior tests for both forges with a later unpushed commit on the same branch, alongside the direct-PR refusal cases.

Forgejo arrived with the upstream merge and deliberately keeps its old registration-time HEAD gate. Without the carve-out, it would lose its only publication check. Extending the stacked-PR behavior to Forgejo would be a separate follow-up.

The merge-time re-record still records pr= and arms the merge poll before live verification, but it no longer sends a premature "PR ready" line to the parent channel. Ordinary PR registration, the ship done gate, Gerrit, local-only landing, and merge authority keep their existing behavior.

One capacity side effect remains: recording pr= frees a declared project-capacity place. A direct-PR merge attempt that has not registered its PR can now record it before refusing an unpublished PR-branch commit, freeing that place while work remains in the copy. Existing red-check, conflict and draft refusals already leave the record that way.

I merged current upstream main with an ordinary merge, without rebasing or force-pushing. The conflict was in the fm-pr-merge.sh header; I kept the existing publication contract and upstream's Forgejo notes, then preserved Forgejo's old gate explicitly. The merge, PR-registration security, project-capacity and captain-hold lifecycle suites all passed, as did pinned ShellCheck/actionlint and the documentation audience check. The regression cases cover pushed stacked branches, unpublished PR-branch commits, missing or rebased heads, GitLab fork identity, and the suppressed parent ready line. The pipeline also drove 33 runs of the real scripts against a local stand-in forge backed by a real bare Git repository, including before-and-after checks against upstream main. I have not performed a real end-to-end merge against a hosted forge.

Pipeline

Updates from git push no-mistakes

... (3 earlier update rounds omitted to keep the PR body within GitHub's 65536-char limit; full history is in the run log.)

⚠️ **Review** - 2 issues (1 warning, 1 info)

🔧 Fix applied.
3 issues (1 warning, 2 infos) still open:

  • ⚠️ bin/fm-pr-check.sh:169 - The refusal this change sets out to remove is still reachable on Forgejo. Intent: "when the PR being merged has its head on the remote and its checks are green, a later unpushed commit on a different branch in the copy must not block that merge." The merge resolution keeps Forgejo on the registration-time HEAD gate (|| { [ &#34;$PROVIDER&#34; != github ] &amp;&amp; [ &#34;$PROVIDER&#34; != gitlab ]; }), and the Forgejo merge case (bin/fm-pr-merge.sh:1856) never calls require_pr_branch_published. Sequence: Forgejo task in direct-PR mode (or no-mistakes, since Forgejo records no pr_head), part-1 pushed and green, copy HEAD on an unpushed part-2 -> fm-pr-merge.sh -> merge-time fm-pr-check.sh -> fm_dod_accept_ship_done gates the copy's HEAD -> "named head <sha> is unreachable outside the worker copy". tests/fm-pr-merge.test.sh:2452 pins that gate. The carve-out itself is correct: without it Forgejo would lose its only named-head check. So this is a scope decision, and the remedy rather than the defect needs authorization: either accept GitHub/GitLab-only for this PR (the header and docs already state it), or extend the publication check to Forgejo (read .head.ref and fork identity in forgejo_verify_mergeable, call require_pr_branch_published before the tea merge, drop the provider carve-out).
  • ℹ️ bin/fm-dod-lib.sh:709 - Tradeoff of the strict containment check: a direct-PR branch whose history was rewritten on the forge (GitLab's Rebase button, GitHub's "Update with rebase") is refused at merge. The copy's branch tip is no longer an ancestor of the verified head, so require_pr_branch_published reports "named head <tip> could not be verified in pull request head <head>" until the worker resets its local branch to the rebased head. Upstream accepted this sequence only while the copy's remote-tracking refs were stale. A forge-side merge-commit update still passes, because the missing head is fetched by SHA and the tip stays an ancestor. Strict containment is the documented and tested design, so no action is requested.
  • ℹ️ tests/fm-pr-merge.test.sh:2986 - The comment in test_failed_merge_reports_nothing still says "The registration's ready line is a fact of its own; only a merge line would misreport the unlanded merge." That is no longer true: since this branch's earlier commit f8b343b, the merge-time re-record in bin/fm-pr-check.sh:266 sends no PR-ready line, so in this test the parent channel file is never written at all. The same commit updated the two sibling comments (tests/fm-pr-merge.test.sh:2938 and :3007) to say so but left this one behind; it was not introduced by this run's fix round. The assertion under it is still correct. Reword the comment to match the siblings, for example: the merge-time re-record sends no ready line, and a merge line would misreport the unlanded merge. No behaviour change is involved.

🔧 Fix applied.
2 issues (1 warning, 1 info) still open:

  • ⚠️ bin/fm-pr-check.sh:169 - The refusal this change sets out to remove is still reachable on Forgejo. Intent: "when the PR being merged has its head on the remote and its checks are green, a later unpushed commit on a different branch in the copy must not block that merge." The merge resolution keeps Forgejo on the registration-time HEAD gate (|| { [ &#34;$PROVIDER&#34; != github ] &amp;&amp; [ &#34;$PROVIDER&#34; != gitlab ]; }), and the Forgejo merge case (bin/fm-pr-merge.sh:1856) never calls require_pr_branch_published. Sequence: Forgejo task in direct-PR mode (or no-mistakes, since Forgejo records no pr_head), part-1 pushed and green, copy HEAD on an unpushed part-2 -> fm-pr-merge.sh -> merge-time fm-pr-check.sh -> fm_dod_accept_ship_done gates the copy's HEAD -> "named head <sha> is unreachable outside the worker copy". tests/fm-pr-merge.test.sh:2452 pins that gate. The carve-out itself is correct: without it Forgejo would lose its only named-head check. So this is a scope decision, and the remedy rather than the defect needs authorization: either accept GitHub/GitLab-only for this PR (the header and docs already state it), or extend the publication check to Forgejo (read .head.ref and fork identity in forgejo_verify_mergeable, call require_pr_branch_published before the tea merge, drop the provider carve-out).
  • ℹ️ bin/fm-dod-lib.sh:709 - Tradeoff of the strict containment check: a direct-PR branch whose history was rewritten on the forge (GitLab's Rebase button, GitHub's "Update with rebase") is refused at merge. The copy's branch tip is no longer an ancestor of the verified head, so require_pr_branch_published reports "named head <tip> could not be verified in pull request head <head>" until the worker resets its local branch to the rebased head. Upstream accepted this sequence only while the copy's remote-tracking refs were stale. A forge-side merge-commit update still passes, because the missing head is fetched by SHA and the tip stays an ancestor. Strict containment is the documented and tested design, so no action is requested.
✅ **Test** - passed

✅ No issues found.

  • Live validation: ✅ go - 11 of 13 scenarios driven live against the product
Scenario Result Live Evidence
GitHub, direct-PR: the worker registers a green part-1 PR, moves on to an unpushed part-2 branch, and firstmate merges part 1 with bin/fm-pr-merge.sh - the merge lands, bound to part 1's verified head ✅ pass live live-stacked-merge-transcript.txt section A. Base 20dcbf9: exit 1, 'named head <part-2 sha> is unreachable outside the worker copy', no merge command, origin main unchanged. This change: exit 0, forg…
GitLab, direct-PR: same stacked flow - the merge request for part 1 merges, bound with --sha to its verified head ✅ pass live live-stacked-merge-transcript.txt section A. Base: exit 1 with the same 'unreachable outside the worker copy' refusal. This change: exit 0, `glab mr merge 41 -R https://gitlab.example/group/project --…
GitLab merge request from a fork: stacked merge goes through when the copy's part-1 branch tracks the fork (different URL form), and is refused when the fork project cannot be read or the fork branch… ✅ pass live live-stacked-merge-transcript.txt section D: gitlab-fork-stacked exit 0 with --sha bound to part 1 (local upstream ssh://git@GitLab.Example/... matched the reported https URL); gitlab-fork-unreadable-…
Adversarial: the PR's own branch has a commit that was never pushed (GitHub and GitLab, direct-PR) - the merge is refused and nothing is merged ✅ pass live live-stacked-merge-transcript.txt section B: github-own-branch-unpushed and gitlab-own-branch-unpushed both exit 1 with 'named head <fix sha> could not be verified in pull request head <verified sha>'…
Adversarial: the fix sits unpushed on the PR's own branch but a different pushed branch carries it and HEAD is pushed - the merge is still refused ✅ pass live live-stacked-merge-transcript.txt section B: github- and gitlab-fix-only-on-another-remote-branch exit 1 on this change with no merge attempted. The base commit merged this same GitHub setup (exit 0)…
Adversarial variants on GitHub: PR branch amended after push; unpushed fix on part 1 with unpushed part 2 on top; PR branch deleted from the copy with an unpushed HEAD; stacked but the check is red -… ✅ pass live live-stacked-merge-transcript.txt section B: github-own-branch-amended and github-stacked-own-branch-unpushed exit 1 ('could not be verified in pull request head'); github-branch-absent-unpushed-head…
The forge head of the PR is ahead of anything the copy fetched (pushed from elsewhere) while the copy is on an unpushed part 2 - the merge lands bound to the forge head and the copy's refs are not mov… ✅ pass live live-stacked-merge-transcript.txt section E: exit 0, --match-head-commit 2e04b9d... (the head pushed from the second clone), origin main gained part-1.txt and review.txt, and 'worker copy branches,…
no-mistakes mode on GitHub and GitLab: the copy has a later unpushed commit on the PR's own branch - the merge lands the verified forge head and is bound to it ✅ pass live live-stacked-merge-transcript.txt section C: github-no-mistakes-unpushed and gitlab-no-mistakes-unpushed exit 0, merge bound to the verified head, origin main has part-1.txt only. Base merged the GitH…
Forgejo, direct-PR, stacked: the registration-time HEAD gate is kept by design, so the merge is refused and nothing is recorded ✅ pass live live-stacked-merge-transcript.txt section F: exit 1 'named head <part-2 sha> is unreachable outside the worker copy', no pr= recorded, merge poll not armed, no merge call. The tea stand-in only had to…
Secondmate home: a refused merge sends no PR-ready line to the parent channel, and a registered-then-merged PR sends exactly one ready line and one merged line ✅ pass live live-stacked-merge-transcript.txt section G: refused merge left state/parent-replies.status with no lines; the register, move on, merge run left 'done [key=child-pr-stack-a1]: child stack-a1 PR ready:…
Ordinary PR-ready registration with bin/fm-pr-check.sh is unchanged: with an unpushed HEAD it still refuses and records nothing ✅ pass live live-stacked-merge-transcript.txt section I: github-stacked, gitlab-stacked and github-own-branch-unpushed all exit 1 with 'is unreachable outside the worker copy', no pr= and no poll, identical to th…
Recorded worktree is gone: a no-mistakes merge still lands bound to the verified head, a direct-PR merge is refused for the missing worktree, and a task in an ungated mode is not checked ⏸️ untested no The prior payload did not establish a live result for this scenario. It was covered only by the existing automated suites with mocked forge CLIs (the 'fm-pr-merge merges a no-mistakes PR whose worktre…
The same stacked merge issued against a real github.com or GitLab repository rather than a local forge stand-in ⏸️ untested no A real merge needs a throwaway repository with an open pull request and green checks on a real forge. The only credential here is the operator's own gh login, and creating repositories, PRs and merges…
  • bash bin/fm-test-run.sh --json &lt;evidence&gt;/targeted-suites.json tests/fm-pr-merge.test.sh tests/fm-pr-check-security.test.sh tests/fm-project-capacity.test.sh tests/fm-captain-hold-lifecycle.test.sh - total=4 failed=0 skipped_gate=0
  • bash drive-stacked-merge.sh &lt;worktree&gt; &#39;this change&#39; &lt;scenario&gt; for 16 scenarios: github-stacked, github-own-branch-unpushed, github-own-branch-amended, github-stacked-own-branch-unpushed, github-fix-only-on-another-remote-branch, github-forge-head-ahead, github-branch-absent-unpushed-head, github-no-mistakes-unpushed, gitlab-stacked, gitlab-own-branch-unpushed, gitlab-fix-only-on-another-remote-branch, gitlab-no-mistakes-unpushed, gitlab-fork-stacked, gitlab-fork-own-branch-unpushed, gitlab-fork-unreadable-stacked, forgejo-stacked
  • The same driver against the base commit's scripts (git archive 20dcbf91 bin) for github-stacked, gitlab-stacked, github-own-branch-unpushed, github-fix-only-on-another-remote-branch, github-no-mistakes-unpushed, gitlab-no-mistakes-unpushed
  • FM_LIVE_REGISTER_FIRST=1 runs of github-stacked and gitlab-stacked on both the change and the base: register part 1 with bin/fm-pr-check.sh, move on to an unpushed part 2, then bin/fm-pr-merge.sh
  • FM_LIVE_CHECK=FAILURE run of github-stacked: a red check still refuses the stacked merge
  • FM_LIVE_MATE=1 runs in a secondmate lab home: a refused merge, and a register-then-merge, with state/parent-replies.status inspected afterwards
  • FM_LIVE_ENTRY=fm-pr-check.sh runs: ordinary registration with an unpushed HEAD on the change (GitHub, GitLab, own-branch fix) and on the base
  • gh pr view https://github.com/kunchenguid/firstmate/pull/6497 --json headRefName -q .headRefName with the real gh against github.com (read-only) returned fm/fm-merge-stacked-head
  • Read-only curl of gitlab.com's public merge request and project records confirmed the fields the change reads: sha, source_branch, source_project_id, target_project_id, web_url, http_url_to_repo, ssh_url_to_repo
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

…ks on a later branch

A direct-PR worker that stacks several pull requests in one copy moves on to the next part on a new local branch before pushing it. Merging the earlier, pushed and green pull request then failed the named-head gate, because the gate tested the copy's HEAD, which is the later unpushed commit.

On the merge path, fm-pr-check now also accepts when the pull request's own head branch in the copy is the forge's head or is reachable outside the copy. An unpushed commit on that branch is still refused, and arming, the ship done gate, GitLab, Gerrit and local-only are unchanged.
@greptile-apps

greptile-apps Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[High risk] Changes merge-time validation for pull requests across multiple scripts.

The PR appears safe to merge based on the changes since the previous review.

Reviews (8) · Last reviewed commit: "no-mistakes(ci): I fixed the real failur..." · Reviewed by Greptile

Comment thread bin/fm-dod-lib.sh
Comment thread bin/fm-pr-check.sh Outdated
…ng now preserves tracking refs, and publication refusals occur before readiness registration. All four relevant test suites and lint pass. CI and Require no-mistakes reported action_required with zero jobs; those workflow blocks are external to the code
@anconina anconina changed the title fix(bin): allow stacked PR merges with unpushed work on another branch fix(bin): stop later unpushed work in the copy from blocking a green stacked PR merge Oct 6, 2026
Comment thread bin/fm-pr-merge.sh
@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate: reviewed against main e06a46fe. Thanks, @anconina. The stacked-PR problem is real and clearly written up. I approved the fork CI runs. There's one open Greptile P1 on this head, so this is waiting on you.

What I checked: bin/fm-pr-merge.sh gains require_pr_branch_published right before both forge merge calls. In no-mistakes mode it trusts the verified forge head, as before. In direct-PR mode it reads the PR head branch (gh pr view --json headRefName on GitHub, source_branch from the verified view on GitLab). If that branch exists locally, its tip must equal or be an ancestor of the verified head (fm_dod_pr_branch_pushed in bin/fm-dod-lib.sh, with a fetch by SHA that moves no refs). Otherwise it falls back to the old fm_dod_accept_ship_done HEAD gate. bin/fm-pr-check.sh now skips the named-head gate and the parent-channel "PR ready" line when FM_PR_CHECK_MERGE=1. Docs and both test files are updated. No workflow files change, and nothing looks disguised: the gate moves to the PR's own branch and still refuses unpushed commits on it. Head 2d6a5f0b matches the no-mistakes attestation.

What still needs fixing (Greptile P1 on 2d6a5f0b, "Fork branch name collision"): on GitLab, source_branch from a fork MR is only a name. If the worker copy has an unrelated local branch with that name, require_pr_branch_published treats it as the MR's branch, and a green MR gets refused. Please:

  1. Scope the local-branch check to the MR's actual source. For example, use it only when source_project_id equals target_project_id, or when the local branch's upstream is the MR's source remote. Otherwise fall back to the HEAD gate.
  2. Add a GitLab fork-MR regression case with a same-named unrelated local branch.

The no-mistakes review also left one info open: docs/secondmate-parent-channel.md:56 still says fm-pr-merge.test.sh covers the PR-ready line at registration. Worth fixing in the same push.

CI: approved fork runs 37506377985 (CI), plus 37506378002, 37506450717 and 37507470269 (Require no-mistakes). Results pending.

Contract-class: new-default. The goal (a pushed, green PR shouldn't be blocked by later unrelated work) is reasonable. But the tip replaces the specified merge-time gate. Main gates the worker copy's HEAD, and this gates the PR's head branch with a new ancestry rule. It also drops the merge-time "PR ready" line to the parent channel. That's a change to a security-relevant default merge path, not a repair of a broken specified one, so even when ready it goes to the captain rather than auto-merging.

VISION.md, rule by rule

  • One captain, one interface: aligns. Fewer bogus refusals, and no "PR ready" line ahead of a refused merge.
  • Authority is explicit and never inferred: aligns. Merging still needs the configured authority, and the gate still refuses unpublished commits on the PR branch.
  • Scripts own the mechanics, agents own the judgment: aligns. Ancestry and containment are exact git checks.
  • A restart is a non-event: aligns. pr= and the merge poll still record on refusal.
  • Delegation with a spine: aligns. Workers stay supervised, and publication is still proven before anything lands.
  • The fleet outlives any vendor: cannot tell. The GitLab fork case is where the open P1 lives.
  • Scope: aligns. Command-layer merge gate.

Next step: push a fix for the P1. Once a fresh no-mistakes raise and green CI are on the new head, this becomes a captain-decision hold, not an auto-merge.

…GitLab fork branch-name collision), as you asked last round. That fix was never in HEAD 2d6a5f0, so I added it now. The problem: when a GitLab merge request came from a fork, the merge check compared the MR against any local branch in the copy that happened to have the same name as the fork's source branch. That branch is unrelated, so the check could refuse a mergeable MR. Changes in bin/fm-pr-merge.sh: - `gitlab_verify_mergeable` now also reads whether the MR comes from a fork, meaning its source and target project ids differ. It stores that as `FM_PR_MERGE_FORK`. The expected field count goes from 9 to 10. - `require_pr_branch_published` skips the same-named local-branch comparison for a fork MR. It falls through to the existing HEAD gate (`fm_dod_accept_ship_done`) instead. - I updated the header comment to say this. Same-project MRs, GitHub and unreadable-branch refusals behave as before. A payload missing both project ids is treated as same-project, so it gets the stricter check it had before. Test changes in tests/fm-pr-merge.test.sh: - The GitLab MR fixture (`write_mr_json`) now includes `source_project_id` and `target_project_id` (both 1 by default), with a `source_project=` override. - New test `test_gitlab_fork_ignores_a_same_named_local_branch`, which sets up an unrelated local `fm/task-x1` branch in a direct-PR copy: - **Fork case** (`source_project_id` 2): the merge now goes through, bound to `--sha` of the verified head. This case fails without the fix and passes with it. - **Same-project case**: still refused, with "named head <sha> could not be verified in pull request head <head>". Verification: - `tests/fm-pr-merge.test.sh`: 109 ok, rc=0 - `tests/fm-pr-check-security.test.sh`: rc=0 - `bin/fm-lint.sh` and shellcheck on both changed files: clean - `bin/fm-test-run.sh --check-coverage`: ok No code change for CI and Require no-mistakes (ci-1 and ci-3): per your decision they're waiting on the upstream maintainer's fork-workflow approval and aren't caused by this code. I left the changes uncommitted for the pipeline to record
Comment thread bin/fm-pr-merge.sh Outdated
…fuse") with the option (b) you chose: GitLab fork MRs no longer get a special case. In the stacked case, a direct-PR fork MR whose part-1 is pushed and green no longer gets refused because the worker's HEAD is on an unpushed part-2. bin/fm-pr-merge.sh is back to exactly what it was at 2d6a5f0: - removed FM_PR_MERGE_FORK - removed the `fork=` field (the field count is back from 10 to 9) - removed the extra fork condition in require_pr_branch_published - removed the fork clause in the header comment Fork MRs now get the same source-branch check as same-project MRs. tests/fm-pr-merge.test.sh: - removed test_gitlab_fork_ignores_a_same_named_local_branch and its call - kept the source_project_id/target_project_id fields in the write_mr_json fixture, so a test can describe a fork MR - added a `fork` case to test_gitlab_direct_pr_merge_checks_the_source_branch: direct-PR mode, a fork MR (source_project_id 2) whose part-1 is pushed and green, HEAD on an unpushed part-2. It must merge, and the merge must be bound with --sha to part-1's verified head. Verification: - The new fork case fails against the previous script ("gitlab-stacked-fork: a pushed part-1 was refused for a later unpushed branch: expected exit 0, got 1") and passes with the fix. - tests/fm-pr-merge.test.sh: 108 ok, rc=0 (one fewer than before because a test was removed). It prints one harmless "No such file" message from a check that the parent-replies file is absent; this change doesn't touch that code. - tests/fm-pr-check-security.test.sh: rc=0. - bin/fm-lint.sh: clean. `shellcheck -x` on both files: clean. - bin/fm-test-run.sh --check-coverage: ok. As you decided, a fork MR is now refused if the copy happens to have an unrelated local branch with the same name as the fork's source branch. That only blocks a merge (it never lets a bad one through), and there is no code for it. No code change for CI (ci-1) or Require no-mistakes (ci-3): as you decided, they're waiting on the upstream maintainer's approval for the fork workflow and this code doesn't cause them. The changes are left uncommitted for the pipeline to record
…nge, building on the work the timed-out agent left in the run worktree. The CI and Require no-mistakes checks need no code change, as you decided. **The fix, in bin/fm-pr-merge.sh** - `gitlab_verify_mergeable` now also records the MR's source project id when it differs from the target project's (`FM_PR_MERGE_FORK_PROJECT`, new `fork_project=` field; the field count goes from 9 to 10). It reads this from the same MR view it already uses. - New `local_branch_is_pr_source <wt> <branch>`: - Same-project MR (the ids match): it returns 0 straight away. - Fork MR: it returns 0 only when the local branch's upstream remote URL exactly matches the fork's `web_url`, `http_url_to_repo` or `ssh_url_to_repo`. It reads those with `glab api projects/<id>` on the MR's own host. - `require_pr_branch_published` now uses the source-branch ancestry check only when that helper returns 0. Otherwise it falls back to the HEAD gate (`fm_dod_accept_ship_done`). - GitHub is unchanged. I updated the header comment to describe the fork scoping. **Tests, in tests/fm-pr-merge.test.sh** - The fake `glab` answers `api projects/2` with the fork's URLs. - `test_gitlab_direct_pr_merge_checks_the_source_branch` now builds its fork cases with part-1 tracking the fork remote: - `fork`: HEAD is on an unpushed part-2, and the merge goes through, bound with `--sha` to part-1. - `fork-unpushed-fix`: still refused. - Both fork cases assert the fork lookup went to the MR's host. - New `test_gitlab_fork_ignores_an_unrelated_same_named_branch` covers an unrelated local `fm/task-x1` that tracks origin: - Fork MR with HEAD pushed: merges. - Fork MR with HEAD unpushed: refused by the HEAD gate ("named head … is unreachable outside the worker copy"). - Same-project MR: still refused ("named head <sha> could not be verified in pull request head <head>"), as before. - docs/secondmate-parent-channel.md is untouched, as you asked. **Verification** - tests/fm-pr-merge.test.sh: 109 ok, rc=0. - tests/fm-pr-check-security.test.sh: rc=0. - bin/fm-lint.sh: clean. `shellcheck -x` on both changed files: clean. - Regression check against the previous script (HEAD d7d9b9b): - With the previous `bin/fm-pr-merge.sh`, the new namesake test fails: "gitlab-namesake-fork-pushed-head: a fork merge request was refused for an unrelated same-named branch: expected exit 0, got 1". - The updated stacked-fork case also fails there, on its fork-lookup assertion. I left the changes uncommitted so the pipeline records them, as in earlier rounds
Comment thread bin/fm-pr-merge.sh Outdated
…"Fork lookup bypasses branch check") as you instructed. CI and Require no-mistakes need maintainer workflow approval, not a code change, so I left them alone. **What was wrong:** for a GitLab merge request from a fork, if the `glab api projects/<id>` lookup failed, the script treated the local branch with the same name as absent. It then checked the worker copy's HEAD instead. So a pushed HEAD could let the merge through even when a fix on the MR's branch was never pushed. The URL check was also an exact string match, so the same project written as `https://…/`, `ssh://git@…` or `git@host:path` didn't count as a match. **Fix, in bin/fm-pr-merge.sh:** - `local_branch_is_pr_source` now returns one of three results: 0 = this is the MR's source branch, 1 = it is not, 2 = the fork could not be read. - It reads the fork first. If glab fails, the reply isn't valid JSON, or it contains none of `web_url` / `http_url_to_repo` / `ssh_url_to_repo`, it returns 2. - Only after a successful read does it return 1, including for a branch with no upstream. - `require_pr_branch_published` refuses on 2 with "source project <id> could not be read to tell whether local branch <branch> is the merge request's source branch". It never falls back to the HEAD check on an unknown. On 1 it uses the HEAD check (`fm_dod_accept_ship_done`) as before. On 0 it checks the MR branch is contained in the verified head, as before. - New `project_url_key` reduces a URL to the lowercase host plus the path. It drops the user part, a trailing slash and `.git`, and it handles `https://`, `ssh://git@host/path` and `git@host:path`. Both the local remote URL and the fork's URLs go through it before comparing. - I updated the header comment. GitHub and same-project MRs are unchanged. **Tests, in tests/fm-pr-merge.test.sh:** - The fake `glab` can now be made to fail the `api projects/2` call. - `test_gitlab_direct_pr_merge_checks_the_source_branch` has three new stacked-fork cases. part-1 tracks the fork through a different URL form in each, and HEAD is on an unpushed part-2. All three merge, bound with `--sha` to part-1: - `fork-slash`: `https://GitLab.Example/fork-owner/project/` (trailing slash, mixed-case host) - `fork-ssh`: `ssh://git@…/project.git` - `fork-scp`: `git@host:path` without `.git` - `test_gitlab_fork_ignores_an_unrelated_same_named_branch` has three new cases, all with HEAD pushed: - `fork-no-upstream`: the lookup succeeds and the branch has no upstream, so the HEAD check applies and the merge goes through. - `fork-lookup-fails` (glab exits 1) and `fork-lookup-empty` (a 404 reply with no URLs): both are refused with the new error, and no merge is attempted. - The existing cases are unchanged: the plain `fork` case, the unpushed fixes, the unpushed HEAD, and same-project. **Verification:** - tests/fm-pr-merge.test.sh: 109 ok, rc=0. It still prints one harmless "No such file" line for parent-replies.status, from code this change doesn't touch. - tests/fm-pr-check-security.test.sh: rc=0. - bin/fm-lint.sh: clean. `shellcheck -x` on both changed files: clean. - Against the HEAD (0ca990f) script, the new tests fail: - "gitlab-stacked-fork-slash: a pushed part-1 was refused for a later unpushed branch: expected exit 0, got 1" - "gitlab-namesake-fork-lookup-fails: an unreadable fork fell back to the copy's pushed HEAD: expected exit 1, got 0" - `project_url_key` gives the same result for all six URL forms under macOS /bin/bash 3.2. I left the changes uncommitted so the pipeline records them, as in earlier rounds
@anconina

anconina commented Oct 6, 2026

Copy link
Copy Markdown
Author

@kunchenguid thanks for the review. Fork P1 is addressed, new head is 523d4e97883102d673a9f6e473c3aec2d5a4ac6e.

  • GitLab fork MRs: the local branch check now only applies when the MR's source project is the target project, or when the local branch's upstream remote is the MR's source project. The fork's web and clone URLs come from glab api projects/<id> and are compared by host and path, so https, ssh:// and git@host:path forms match with or without a trailing slash or .git. Any other same-named branch falls back to the HEAD gate. If the source project can't be read, the merge refuses instead of falling back.
  • Regression tests in tests/fm-pr-merge.test.sh: an unrelated same-named branch falls back to the HEAD gate, a stacked fork MR merges while the worker is on an unpushed later part (and still refuses with an unpushed fix on the MR branch), same-project MRs are unchanged, URL-form variants match, and a failed source-project lookup refuses.
  • docs/secondmate-parent-channel.md:56: this was already corrected in 2d6a5f0b; it now credits tests/fm-teardown.test.sh for the PR-ready line at registration and tests/fm-pr-merge.test.sh for the merge-time re-record sending none.

CI and Require no-mistakes are waiting on workflow approval again for the new head.

@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate: re-reviewed head 523d4e97 against main e06a46fe. Thanks, @anconina. The GitLab fork P1 is addressed, and this is now waiting on CI.

What changed since my last look (2d6a5f0b):

  • gitlab_verify_mergeable now also reads source_branch and fork_project from the same verified MR view. It still requires every field exactly once (10 of 10).
  • For a fork MR, local_branch_is_pr_source reads the source project through glab api projects/<id> (read-only). It counts the local branch only when its upstream remote URL matches the fork's web, http, or ssh URL by lowercase host and path. Any other same-named branch falls back to the HEAD gate, and a source project that can't be read refuses the merge. Both behaviours are the right way to fail closed.
  • New regression cases cover an unrelated same-named branch, a stacked fork MR with an unpushed later part, an unpushed fix on the MR branch that still refuses, same-project MRs left unchanged, URL-form variants, and a failed lookup.
  • The docs/secondmate-parent-channel.md note was already fixed in 2d6a5f0b. Thanks for pointing that out.

There are no workflow edits and no new network calls beyond the read-only glab api. Greptile is green on this head, and the no-mistakes attestation matches it.

CI: approved fork runs 37533628372 (CI), plus 37533628387 and 37534187919 (Require no-mistakes). Results pending.

Contract-class: new-default (unchanged). The tip replaces the specified merge-time publication gate. Main checks the worker copy's HEAD, and this checks the PR's own head branch with an ancestry rule. It also stops the merge-time re-record from sending the parent "PR ready" line. That makes it a captain call, not an auto-merge.

VISION.md, rule by rule

  • One captain, one interface: aligns. There are fewer bogus refusals, and no "PR ready" line comes before a refused merge.
  • Authority is explicit and never inferred: aligns. The configured merge authority is unchanged, and unpublished commits on the PR branch still refuse.
  • Scripts own the mechanics, agents own the judgment: aligns. Ancestry and URL matching are exact checks.
  • A restart is a non-event: aligns. pr= and the merge poll still record on refusal.
  • Delegation with a spine: aligns. Publication is still proven before anything lands.
  • The fleet outlives any vendor: aligns now. GitLab fork MRs are bound to their actual source project.
  • Scope: aligns. This is the command-layer merge gate.

Next step: once CI and Require no-mistakes are green on 523d4e97, this becomes a captain-decision hold (merge or leave), not something waiting on you.

@anconina

anconina commented Oct 7, 2026

Copy link
Copy Markdown
Author

@kunchenguid thanks for the re-review. CI on 523d4e97 has one failure caused by this PR: tests/fm-captain-hold-lifecycle.test.sh ("the PR merge entrypoint refused a home carrying no backlog"). The new merge-time head-branch check refuses a case that main still merges. The fix and a regression test are in progress. I'll push a new head shortly, and the fork runs will need approval again.

The other failure, tests/fm-remote-reply.test.sh ("the NUL-bearing line was not captured"), is in code this PR doesn't touch.

…lel 2", following your round-6 instructions. "Behavior portable serial 4" failed on something else that this PR doesn't touch. **The failure in parallel 2:** tests/fm-captain-hold-lifecycle.test.sh failed with "the PR merge entrypoint refused a home carrying no backlog". Its fixture records kind=ship, mode=ship and a worktree that doesn't exist. `require_pr_branch_published` in bin/fm-pr-merge.sh only skipped no-mistakes mode. For every other mode it ran `cd "$wt" && gh pr view`, which fails when the worktree is gone, so it refused with "pull request head branch could not be verified". Main never checks this case, because `fm_dod_should_gate_ship_done` does not gate that mode. I reproduced the failure by running just that test locally. **Fix in bin/fm-pr-merge.sh (`require_pr_branch_published`):** 1. It now returns 0 when `fm_dod_should_gate_ship_done` would not gate the task's kind, mode and `done: PR <url>` line. This sits next to the existing no-mistakes early return. 2. GitHub's headRefName is read by URL (`gh pr view "$URL" --json headRefName`) without going into the worktree. It is still required and validated. 3. The local-branch ancestry check only runs when the worktree is an existing directory with that branch. Otherwise it goes through `fm_dod_accept_ship_done` as main does, so a direct-PR task with a missing worktree is still refused with "worktree missing". The same rule applies to GitLab. The guard also stops `git -C ""` from quietly running in the current directory when the recorded worktree is empty. 4. I updated the header comment to match. **Tests:** - tests/fm-captain-hold-lifecycle.test.sh: the fake gh now answers `--json headRefName`. With fix 1 the mode=ship fixture returns before it ever asks, so this answer isn't exercised today. - tests/fm-pr-merge.test.sh: new `test_missing_worktree_merge_follows_the_mode`. A no-mistakes merge with the recorded worktree gone still merges, bound with `--match-head-commit`. A direct-PR one is refused with "named head cannot be verified: worktree missing" and no merge is attempted. - With the previous script the direct-PR case fails ("the refusal did not name the missing worktree"). The no-mistakes case passed before too, since that early return was already there. The captain-hold test is the one that reproduces the CI failure. **Verification:** - tests/fm-captain-hold-lifecycle.test.sh: 61 ok, rc=0, including the case that failed in CI. - tests/fm-pr-merge.test.sh: 110 ok, rc=0. It still prints three "No such file" lines from `assert_no_grep` on parent-replies.status, which the merge-time re-record no longer writes after the earlier R5-1 change. These are harmless, and the assertions pass. - tests/fm-pr-check-security.test.sh: rc=0. - bin/fm-lint.sh, `shellcheck -x` on the changed files, and `bin/fm-test-run.sh --check-coverage`: clean. **Not changed:** - Serial 4 failed in tests/fm-remote-reply.test.sh ("the NUL-bearing line was not captured"). That's a timed wait in a test that doesn't use fm-pr-merge, fm-pr-check or fm-dod-lib, so I left it alone as you said. - Require no-mistakes needs nothing. I left the changes uncommitted so the pipeline records them, as in earlier rounds
@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate: re-reviewed new head c579a309 against main 237e1cf3. Thanks, @anconina, for tracking down the CI failure. This is waiting on CI again.

What changed since my last stamp (523d4e97): require_pr_branch_published now returns early when fm_dod_should_gate_ship_done would not gate the task, matching main for modes main never checked. GitHub's headRefName is now read by URL, so a missing worktree no longer breaks the read. The local-branch ancestry check runs only when the recorded worktree exists and has that branch, and otherwise falls back to fm_dod_accept_ship_done. This also stops git -C "" from silently running in the current directory. The new test_missing_worktree_merge_follows_the_mode covers both sides: a no-mistakes merge still lands, bound with --match-head-commit, and a direct-PR merge still refuses with "worktree missing". The captain-hold lifecycle fixture also answers headRefName now. I agree the fm-remote-reply serial failure is outside this PR. It is 4 commits behind main and still mergeable. There are no workflow edits and no new network calls. Greptile is green on this head, and the no-mistakes attestation matches c579a309.

CI: approved fork runs 37574793468 (CI), plus 37574793355 and 37575071855 (Require no-mistakes). All are queued.

Contract-class: new-default (unchanged). The tip replaces the specified merge-time publication gate. Main checks the worker copy's HEAD, and this checks the PR's own head branch with an ancestry rule. It also stops the merge-time re-record from sending the parent "PR ready" line. That makes it a captain call, not an auto-merge.

VISION.md, rule by rule

  • One captain, one interface: aligns. There are fewer bogus refusals, and no "PR ready" line comes before a refused merge.
  • Authority is explicit and never inferred: aligns. The configured merge authority is unchanged, and unpublished commits on the PR branch, or a direct-PR task whose copy is gone, still refuse.
  • Scripts own the mechanics, agents own the judgment: aligns. Ancestry, URL matching and the mode gate are exact checks.
  • A restart is a non-event: aligns. pr= and the merge poll still record on refusal.
  • Delegation with a spine: aligns. Publication is still proven before anything lands.
  • The fleet outlives any vendor: aligns. GitLab fork MRs are bound to their actual source project.
  • Scope: aligns. This is the command-layer merge gate.

Next step: once CI and Require no-mistakes are green on c579a309, this becomes a captain-decision hold (merge or leave), not something waiting on you.

@anconina

anconina commented Oct 9, 2026

Copy link
Copy Markdown
Author

@kunchenguid CI is green on c579a309 now, including the fm-captain-hold-lifecycle case that failed last time. Whenever you have a minute, it should be ready for another look. Thanks!

@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate: thanks @anconina, and thanks for the nudge. You're right: CI and both Require no-mistakes runs are green on c579a309, including the fm-captain-hold-lifecycle case that failed before.

One thing has changed since then. Main moved on, and GitHub now reports this branch as conflicting with main (b75658b6). The overlap is bin/fm-pr-check.sh, which #5343 (opt-in per-project spawn capacity) changed on 10-07 after your last push.

Contract-class: new-default (unchanged). The tip replaces the specified merge-time publication gate: main checks the worker copy's HEAD, and this checks the PR's own head branch with an ancestry rule. It also stops the merge-time re-record from sending the parent "PR ready" line. Because it's a captain decision and not an auto-merge, I won't rebase it on your behalf. That part stays with you.

VISION.md, rule by rule (unchanged from my 10-07 review; no code moved since): one captain, one interface: aligns (fewer bogus refusals, no "PR ready" before a refused merge). Authority: aligns (merge authority unchanged; unpublished commits on the PR branch, or a direct-PR task whose copy is gone, still refuse). Scripts own the mechanics: aligns. A restart is a non-event: aligns. Delegation with a spine: aligns (publication still proven before anything lands; live validation go 10 of 14). The fleet outlives any vendor: aligns. Scope: aligns.

What would help (on you), whenever you have time: merge or rebase onto current main, resolve bin/fm-pr-check.sh against #5343, and re-raise through git push no-mistakes so the attestation binds the new head. I'll approve the fork runs when they appear.

Next step: waiting on you for the rebase. Once it's green and mergeable, it goes to Kun as a merge-or-leave decision, and from then on it isn't waiting on you.

@anconina anconina changed the title fix(bin): stop later unpushed work in the copy from blocking a green stacked PR merge fix(bin): gate PR merges on the pull request's own branch instead of the worker copy's HEAD Oct 10, 2026
@anconina

Copy link
Copy Markdown
Author

@kunchenguid thanks for the heads-up about the conflict. The branch is up to date now. New head: 0d2680640be514c42350b137e593f12b60b08b23.

  • I merged current main into the branch with an ordinary merge commit, so there was no rebase and no force-push.
  • The only conflict was the header comment at the top of bin/fm-pr-check.sh. I kept both sides: feat(bin): defer spawns beyond a declared per-project capacity, opt-in #5343's note that the recorded pr= frees the task's place in a declared project capacity, and this PR's note that a merge-time re-record skips the named-head gate. No code lines conflicted.
  • I re-raised through git push no-mistakes, so the attestation now binds 0d268064. The review, test, document and lint steps all completed on that head. The test step hit its time budget once while the machine was overloaded and passed on the rerun.

CI and Require no-mistakes are waiting on fork workflow approval for the new head.

@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate: thanks @anconina, this is just what was needed. I checked the merge commit 0d268064: it merges current main with no rebase, and the only overlap with #5343 is the header comment in bin/fm-pr-check.sh, where both notes are kept. No workflow files are touched. I approved the fork runs for this head: CI 38073758071 and Require no-mistakes 38074402555.

Contract-class: new-default (unchanged). The merge-time publication gate now checks the PR's own head branch instead of the worker copy's HEAD. That replaces a specified default path, so it goes to Kun and won't be auto-merged.

VISION.md per rule (unchanged; this push changes no logic): one captain, one interface — aligns. Authority — aligns (merge authority is unchanged, and unpublished commits still refuse). Scripts own the mechanics — aligns. A restart is a non-event — aligns. Delegation with a spine — aligns. Fleet outlives any vendor — aligns (GitHub and GitLab). Scope — aligns.

Next step: waiting on CI. Nothing more is needed from you right now. Once CI and no-mistakes are green and the PR is mergeable, it goes to Kun as a merge-or-leave decision.

@anconina

Copy link
Copy Markdown
Author

CI is back on 0d268064: 18 checks passed and one, "Behavior portable serial 2", was cancelled when it ran into the job's 30-minute limit. It didn't fail a test.

Looking at the log for that job, the four tests that finished all exited 0. tests/fm-supervision-host.test.sh took about 28 of the 30 minutes on its own, and tests/fm-procevent-when.test.sh had just printed "all fm-procevent-when tests passed" when the job was cut. This PR doesn't touch either of those tests or bin/fm-supervision-host.sh. The same shard was cancelled at the limit on main in run 37737126056, and on the recent main runs I looked at it takes 17 to 23 minutes, so it runs fairly close to the limit.

I can't re-run the job from the fork. Could you re-run "Behavior portable serial 2" on run 38073758071 before merging, so it has a real verdict on this head?

@anconina anconina changed the title fix(bin): gate PR merges on the pull request's own branch instead of the worker copy's HEAD fix(bin): stop later unpushed work in the copy from blocking a green stacked PR merge Oct 10, 2026
@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate: thanks @anconina, and thanks for reading the log. You're right: the four tests that finished all exited 0, tests/fm-supervision-host.test.sh used about 28 of the 30 minutes, and main's own serial 2 has been cancelled at the limit the same way. I've re-run the failed job of run 38073758071, so this head gets a real serial 2 verdict.

Head 0d268064 is unchanged, so my verdict stands: contract-class new-default (the merge-time gate now checks the PR's own head branch instead of the worker copy's HEAD), and every VISION.md rule aligns.

Next step: waiting on CI only. Nothing more is needed from you. Once serial 2 is green and the PR is still mergeable, it goes to Kun as a merge-or-leave decision. Triage won't auto-merge it.

@anconina anconina changed the title fix(bin): stop later unpushed work in the copy from blocking a green stacked PR merge fix(bin): merge a pushed stacked PR while the copy sits on a later unpushed branch Oct 12, 2026
@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate: thanks @anconina for merging main and working through the Forgejo interaction.

What changed since my 2026-10-10 stamp (0d268064 → fb2a5ca2): a merge of main 20dcbf91, which brings #4395 Forgejo/tea, plus two review commits. I compared the PR's own patch before and after. The resolution scopes the merge-time skip in fm-pr-check.sh to GitHub and GitLab, so Forgejo keeps its registration-time named-head gate. It also adds tests that pin a no-mistakes merge to the verified forge head while a direct-PR merge refuses the unpushed fix. That narrows the change rather than widening it. There's no workflow, network or secret surface. The attestation matches fb2a5ca2 (live go 11/13).

CI: the fork runs were action_required. I approved them after the diff review: 38285122678 (CI), and 38285122664, 38285180951, 38285310983 (Require no-mistakes).

Contract-class (mine, unchanged): new-default. The merge-time gate checks the PR's own head branch instead of the worker copy's HEAD.

VISION.md per rule (unchanged): all rules align; delegation and vendor neutrality are, if anything, strengthened by the explicit Forgejo handling.

Next: waiting on CI only. Nothing is needed from you. Once it's green and still mergeable, it goes to Kun as a merge-or-leave decision. Triage won't auto-merge it.

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.

2 participants