feat(forge): mergePullRequest + review-state parity across forges (MF-8) #313

Merged

code-lead merged 1 commit from feat/299-mf8-merge-review-parity into main

2026-04-24 10:00:06 +00:00

claude-desktop commented

2026-04-24 09:50:04 +00:00

Collaborator

Summary

Adds ForgePort.mergePullRequest(repo, prNumber, opts) and implements it on all three adapters so domain code can squash-merge a PR without caring which forge hosts it.
Migrates review/merge-flow ForgejoAdapter callsites in webhook-ci.ts + review-loop.ts onto createForgeAdapterForRepo(...) — a GitHub- or GitLab-hosted PR now picks up the correct adapter transparently. No-op for forgejo-bound repos.

Design notes

Per-adapter merge mapping:

Forgejo: POST /pulls/{n}/merge with Do: "squash", title/message forwarded as MergeTitleField / MergeMessageField.
GitHub: PUT /pulls/{n}/merge with merge_method: "squash", title/message as commit_title / commit_message.
GitLab: PUT /merge_requests/{iid}/merge with squash: true, title/message as squash_commit_message / merge_commit_message.

Squash-only contract: per MF-8 spec "Out of scope" — rebase-merge, merge-commit and merge queues are not supported. opts.squash is accepted for port symmetry but every adapter always sends a squash; documenting it in the port doc so the shape reads naturally at callsites without implying non-squash strategies are wired.

Return contract: true on any 2xx, false on non-2xx (405 "not mergeable", 409 "SHA out of date", auth failure, transport error). Callers that need to distinguish cases can probe getPullRequest for merged / mergeable.

Review-state parity: audited the consuming code in review-loop.ts + webhook-ci.ts + webhook-handlers.ts. The domain already speaks the ReviewState union ("approved", "changes_requested", "comment", …) — only the adapters' toReviewState mappers touch the raw APPROVED / CHANGES_REQUESTED / COMMENTED strings, which is correct. Remaining "APPROVED" / "REQUEST_CHANGES" string literals in the review flow are log/comment prose, not state checks.

Force-merge path: confirmed — the round-cap counter and pill-red logic are forge-agnostic (countChangeRequestRounds + guardAuthorDispatch), and the terminal dispatchMerge(…, { force: true }) call still hands off to the boss agent which performs the squash through MCP. No direct host-side merge call exists on the force-merge path today; mergePullRequest is now available for any future automation that wants to bypass the agent-dispatch indirection.

Test plan

bun test apps/server/src/infrastructure/forge/ — 182 pass (adds 12 new mergePullRequest assertions across Forgejo/GitHub/GitLab)
bun test apps/server/src/domain/workflow/review-loop.test.ts apps/server/src/domain/workflow/force-merge.integration.test.ts apps/server/src/http/webhook-ci.test.ts apps/server/src/http/webhook-handlers.test.ts apps/server/src/http/webhook.test.ts — 137 pass
bun x turbo run typecheck clean across all 4 packages
bun x biome check clean
Live smoke on a forgejo PR after merge — no-op migration, but worth eyeballing one real CI-green → review-requested → approval cycle to confirm the factory default still routes through ForgejoAdapter

Out of scope (flagged)

submitReview port addition — mentioned in the MF-8 spec narrative but not in the acceptance criteria; the reviewer agent still submits verdicts via MCP (create_pull_review / submit_pull_review). Tracked for a follow-up once a host-side code path actually needs to submit reviews without going through an agent.
Remaining new ForgejoAdapter(token) callsites in janitor.ts, deps.ts, pipeline.ts, board.ts, foreman.ts, webhook-handlers.ts, slash-commands.ts, main.ts. Not on the review/merge critical path — touching them here would blow scope. Recommend a dedicated MF-10 story to finish the factory migration across the whole service.
Dashboard-PR forge binding: the dashboard assumes forge.jacquin.app URLs in a few places (PR links, review URLs). Display-layer follow-up, not review-flow parity.

Closes #299

## Summary - Adds `ForgePort.mergePullRequest(repo, prNumber, opts)` and implements it on all three adapters so domain code can squash-merge a PR without caring which forge hosts it. - Migrates review/merge-flow `ForgejoAdapter` callsites in `webhook-ci.ts` + `review-loop.ts` onto `createForgeAdapterForRepo(...)` — a GitHub- or GitLab-hosted PR now picks up the correct adapter transparently. No-op for forgejo-bound repos. ## Design notes **Per-adapter merge mapping**: - Forgejo: `POST /pulls/{n}/merge` with `Do: "squash"`, title/message forwarded as `MergeTitleField` / `MergeMessageField`. - GitHub: `PUT /pulls/{n}/merge` with `merge_method: "squash"`, title/message as `commit_title` / `commit_message`. - GitLab: `PUT /merge_requests/{iid}/merge` with `squash: true`, title/message as `squash_commit_message` / `merge_commit_message`. **Squash-only contract**: per MF-8 spec "Out of scope" — rebase-merge, merge-commit and merge queues are not supported. `opts.squash` is accepted for port symmetry but every adapter always sends a squash; documenting it in the port doc so the shape reads naturally at callsites without implying non-squash strategies are wired. **Return contract**: `true` on any 2xx, `false` on non-2xx (405 "not mergeable", 409 "SHA out of date", auth failure, transport error). Callers that need to distinguish cases can probe `getPullRequest` for `merged` / `mergeable`. **Review-state parity**: audited the consuming code in `review-loop.ts` + `webhook-ci.ts` + `webhook-handlers.ts`. The domain already speaks the `ReviewState` union (`"approved"`, `"changes_requested"`, `"comment"`, …) — only the adapters' `toReviewState` mappers touch the raw `APPROVED` / `CHANGES_REQUESTED` / `COMMENTED` strings, which is correct. Remaining `"APPROVED"` / `"REQUEST_CHANGES"` string literals in the review flow are log/comment prose, not state checks. **Force-merge path**: confirmed — the round-cap counter and pill-red logic are forge-agnostic (`countChangeRequestRounds` + `guardAuthorDispatch`), and the terminal `dispatchMerge(…, { force: true })` call still hands off to the boss agent which performs the squash through MCP. No direct host-side merge call exists on the force-merge path today; `mergePullRequest` is now available for any future automation that wants to bypass the agent-dispatch indirection. ## Test plan - [x] `bun test apps/server/src/infrastructure/forge/` — 182 pass (adds 12 new `mergePullRequest` assertions across Forgejo/GitHub/GitLab) - [x] `bun test apps/server/src/domain/workflow/review-loop.test.ts apps/server/src/domain/workflow/force-merge.integration.test.ts apps/server/src/http/webhook-ci.test.ts apps/server/src/http/webhook-handlers.test.ts apps/server/src/http/webhook.test.ts` — 137 pass - [x] `bun x turbo run typecheck` clean across all 4 packages - [x] `bun x biome check` clean - [ ] Live smoke on a forgejo PR after merge — no-op migration, but worth eyeballing one real CI-green → review-requested → approval cycle to confirm the factory default still routes through `ForgejoAdapter` ## Out of scope (flagged) - **`submitReview` port addition** — mentioned in the MF-8 spec narrative but not in the acceptance criteria; the reviewer agent still submits verdicts via MCP (`create_pull_review` / `submit_pull_review`). Tracked for a follow-up once a host-side code path actually needs to submit reviews without going through an agent. - **Remaining `new ForgejoAdapter(token)` callsites** in `janitor.ts`, `deps.ts`, `pipeline.ts`, `board.ts`, `foreman.ts`, `webhook-handlers.ts`, `slash-commands.ts`, `main.ts`. Not on the review/merge critical path — touching them here would blow scope. Recommend a dedicated MF-10 story to finish the factory migration across the whole service. - **Dashboard-PR forge binding**: the dashboard assumes `forge.jacquin.app` URLs in a few places (PR links, review URLs). Display-layer follow-up, not review-flow parity. Closes #299

claude-desktop added 1 commit

2026-04-24 09:50:05 +00:00

feat(forge): mergePullRequest + review-state parity across forges (MF-8)

qa / qa (pull_request) Successful in 3m53s

Details

qa / dockerfile (pull_request) Successful in 8s

Details

69da7a6823

Adds `ForgePort.mergePullRequest(repo, prNumber, opts)` and implements it
on all three adapters so domain code can squash-merge a PR without
caring which forge hosts it:

- Forgejo: `POST /pulls/{n}/merge` with `Do: "squash"` (title/message
  forwarded as `MergeTitleField` / `MergeMessageField`).
- GitHub: `PUT /pulls/{n}/merge` with `merge_method: "squash"`
  (title/message forwarded as `commit_title` / `commit_message`).
- GitLab: `PUT /merge_requests/{iid}/merge` with `squash: true`
  (title/message forwarded as `squash_commit_message` /
  `merge_commit_message`).

Per MF-8 spec the squash contract is the project convention — rebase
merge, merge-commit and merge queues stay out of scope. `opts.squash`
is accepted for port symmetry but the adapter always sends a squash.

Also migrates the review/merge-flow `new ForgejoAdapter(token)`
instances in `webhook-ci.ts` and `review-loop.ts` onto
`createForgeAdapterForRepo(repo, token)` so a PR hosted on GitHub /
GitLab picks up the correct adapter transparently. This is a no-op
for forgejo-bound repos (factory defaults to Forgejo) and closes the
review-flow parity gap from the MF-8 acceptance criteria.

Out of scope (flagged as follow-ups): `submitReview` port addition
mentioned in the spec narrative, and the remaining `new ForgejoAdapter`
callsites in janitor / deps / pipeline / board / foreman / handlers —
those are not on the review/merge critical path and touching them
would blow this PR's scope.

Closes #299