Skip to content

feat(pr): add GitLab merge and teardown discovery support - #1388

Open
es-nflange wants to merge 1 commit into
kunchenguid:mainfrom
es-nflange:fm/fm-gitlab-delivery
Open

feat(pr): add GitLab merge and teardown discovery support#1388
es-nflange wants to merge 1 commit into
kunchenguid:mainfrom
es-nflange:fm/fm-gitlab-delivery

Conversation

@es-nflange

Copy link
Copy Markdown

Summary

Firstmate could deliver and land work on GitHub but not GitLab, even though bin/fm-pr-lib.sh already parsed GitLab merge request URLs for the watcher. This closes the gap so GitLab-hosted projects have a sanctioned way to land approved work.

  • bin/fm-pr-merge.sh (primary fix): adds a GitLab branch that merges via glab mr merge, addressed by the merge request's full host-qualified project URL (https://<host>/<group path>) rather than owner/repo, since a GitLab project can sit several groups deep. Records pr= before merging exactly as before. Defaults to --squash, adds --yes --auto-merge=false so the merge happens immediately and deterministically rather than deferring to a pipeline-success auto-merge. Every existing GitHub behavior, flag, and refusal is unchanged.
  • bin/fm-teardown.sh: the landed-work check is now provider-aware. When pr= is recorded, the provider comes from parsing that URL. When it isn't (e.g. a yolo merge with no PR CI), the provider is guessed from the project's own origin remote and the merge request is discovered from the branch name via glab mr list. GitLab's merge state comes from glab mr view's plain field output (no JSON processor needed, matching fm-pr-poll.sh's existing approach), and its head commit comes from GitLab's own refs/merge-requests/<n>/head git ref rather than a JSON field. Every lookup failure or ambiguity falls back to the existing content-in-default check exactly as before; nothing here can cause teardown to assume landed when it can't determine the merge state (fail-safe preserved).
  • bin/fm-brief.sh: the generated Rules and direct-PR definition-of-done now match the project's actual forge (glab/"merge request" vs gh-axi/"PR"), derived from the clone's own origin remote. Falls back to the GitHub wording when the remote can't be read or parsed.
  • Docs: bin/fm-project-mode.sh, AGENTS.md section 3, docs/architecture.md, docs/scripts.md, and docs/gitlab-merge-watch.md updated to reflect GitLab as a first-class delivery path.

Known remaining gap

tasks-axi done <id> --pr <url> (used by fm-teardown.sh's backlog-refresh reminder) rejects GitLab merge-request URLs; it wants /pull/<n>. That's in the external tasks-axi npm package, not this repo, so it's left as-is per the task instructions.

Testing

  • tests/fm-pr-merge.test.sh: added 3 new tests covering the GitLab merge path, including a merge request whose project path sits 4 groups deep, an explicit-merge-method (--rebase) case, and a merge-failure-propagates case. Updated the "malformed URL" test to use a URL invalid under both providers (a GitLab URL is no longer inherently malformed). All 13 tests pass, including every pre-existing GitHub test unchanged.
  • tests/fm-teardown.test.sh: added 3 new tests - a GitLab pr= recorded and merged (allow), a GitLab merge request discovered by branch name with no pr= recorded (allow, the real-world scenario from the motivating epic), and a GitLab lookup error with unlanded content (refuse, fail-safe). All 36 tests pass.
  • tests/fm-pr-check-security.test.sh: updated the one assertion that pinned the old "refuses a GitLab URL" behavior to assert the new merge behavior instead. All 36 tests pass.
  • tests/fm-brief.test.sh: all 17 pre-existing tests pass unchanged.
  • bin/fm-lint.sh (shellcheck, pinned 0.11.0): clean on every touched file and on the full repo.
  • bin/fm-doc-audience-check.sh: clean.
  • Manually verified against the real glab CLI (installed, authenticated) that -R/--repo accepts a full host-qualified URL for arbitrarily deep group paths, and that a bare group/path argument resolves against glab's own configured default host rather than the merge request's actual host - this is why fm-pr-merge.sh always passes the full URL.
  • Ran bin/fm-test-run.sh --changed --base origin/main (30 files selected as touching the affected surface) and bin/fm-test-run.sh --all (full 101-file suite) in the background; both were still in progress at the time of this PR. The only failures observed anywhere in the full-suite run belong to the unrelated real-herdr-gated family (live Herdr workspace/session e2e tests, e.g. "primary-shaped crewmate spawn failed", "startup workspace shell did not become ready") - these touch none of the files changed here and are consistent with known flakiness running Herdr e2e tests nested inside a live session. No failures were observed in any file this PR touches.

Not tested: no real merge was performed against a live GitLab instance (explicitly out of scope - never merge anything real). The glab mr merge invocation shape is verified by argument-capture mocks plus manual --repo/-R behavior checks against the real CLI, not by an actual merge.

fm-pr-merge.sh only merged GitHub PRs, so operators whose projects are
GitLab-hosted had no sanctioned way to land approved work through the
merge helper AGENTS.md requires for every task merge. Add a GitLab
branch that merges via glab, addressed by the merge request's full
host-qualified project path since GitLab projects can sit arbitrarily
deep under groups.

fm-teardown.sh's landed-work check gets the same treatment: it now
resolves a GitLab merge request's state and head (via glab and GitLab's
own refs/merge-requests/<n>/head ref) both from a recorded pr= and, when
none was recorded, by guessing the provider from the project's origin
remote and discovering the merge request from the branch name - always
failing safe to the existing content check on any lookup uncertainty.

fm-brief.sh now derives the project's forge from its origin remote and
generates matching tool guidance (glab/merge request vs gh-axi/PR)
instead of unconditionally telling every worker to use gh-axi.
@es-nflange

Copy link
Copy Markdown
Author

Follow-up on the testing note above, now that the background full-suite runs have progressed further.

  • bin/fm-test-run.sh --changed --base origin/main (30 files) surfaced 2 failures in tests/fm-test-run.test.sh itself ("scheduler waited for oldest worker", "jobs=2 must refill the first completed slot") - these are wall-clock concurrency-scheduler self-tests of the test runner. Re-ran that file in isolation (not competing with the concurrently-running --all background job for CPU) and all 15 of its tests passed, confirming this was resource contention from running two large suites at once on this machine, not a regression. Nothing in this PR touches bin/fm-test-run.sh or its scheduler.
  • bin/fm-test-run.sh --all (full 101-file suite) is still running. So far every failure observed (fm-hibit-resume-r1 same-identity reclaim, startup workspace shell did not become ready, primary-shaped crewmate spawn failed, restoring the SSHHIP child did not clear only its narrow warning) is in the unrelated real-herdr-gated or snapshot-bearings families - live Herdr/tmux workspace e2e tests, none of which import or exercise fm-pr-merge.sh, fm-pr-lib.sh, fm-teardown.sh, or fm-brief.sh.
  • Every test file this PR actually touches or that exercises the changed code paths (fm-pr-merge.test.sh, fm-teardown.test.sh, fm-brief.test.sh, fm-pr-check-security.test.sh) passes in full, individually and confirmed again just now.

I won't hold this PR open for the remainder of the --all run to finish (it's a multi-hour run dominated by unrelated live-session e2e tests); happy to report final numbers if asked.

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.

1 participant