From 5a37f2d413953055f1acdd1eb4a7d71d1c899a48 Mon Sep 17 00:00:00 2001 From: SJ Lee Date: Mon, 21 Sep 2026 14:21:42 +0000 Subject: [PATCH] fix(relay): reject GitLab MRs targeting different base (Closes #1339) Reject merge requests whose target is not the requested base before identity-match or exact-head bind so a same-source-branch MR is never reused against a different base. Closes #1339 --- .../forge-change-request-adapters.md | 10 +- .../relay-dispatch/scripts/forge-adapters.js | 17 ++- .../scripts/forge-adapters.test.js | 139 ++++++++++++++++++ 3 files changed, 156 insertions(+), 10 deletions(-) diff --git a/skills/relay-dispatch/references/forge-change-request-adapters.md b/skills/relay-dispatch/references/forge-change-request-adapters.md index 7c8dd00f..bc8b79d5 100644 --- a/skills/relay-dispatch/references/forge-change-request-adapters.md +++ b/skills/relay-dispatch/references/forge-change-request-adapters.md @@ -104,9 +104,13 @@ Native GitLab merge-request observation through the GitLab REST API (`source_project_id === target_project_id`) identity-match, mirroring the GitHub head-repo identity requirement. Fork MRs are counted in `fork_mr_count` and never selected implicitly. -- **Selection**: the same decision ladder as the GitHub route — identity - matches win, a recorded closed iid is adoptable, exact-head matches beat - looser pools, and only a unique candidate is selected. +- **Selection**: reject MRs whose `target_branch` is not the requested + `baseBranch` before identity-match or exact-head bind. After that, the + same ladder as the GitHub route — identity matches win, a recorded closed + iid is adoptable, exact-head matches beat looser pools, and only a unique + candidate is selected. A same-source-branch MR targeting a different base + is not reused; no match is an empty lookup (`matching_mr_count: 0`) so a + new MR can still be created. - **Typed failures**: `GITLAB_AUTH_INVALID` (401), `GITLAB_PERMISSION_DENIED` (403), `GITLAB_NOT_FOUND` (404), `GITLAB_REQUEST_REJECTED` (other 4xx), `GITLAB_OUTAGE` (429/5xx/network, `retryable: true`), and diff --git a/skills/relay-dispatch/scripts/forge-adapters.js b/skills/relay-dispatch/scripts/forge-adapters.js index 8cd0c4fd..ff3a1e0f 100644 --- a/skills/relay-dispatch/scripts/forge-adapters.js +++ b/skills/relay-dispatch/scripts/forge-adapters.js @@ -192,16 +192,19 @@ function sameGitlabProjectMr(row) { return row?.source_project_id != null && row.source_project_id === row.target_project_id; } -// Mirrors the retained selectGithubPr decision ladder for GitLab facts: -// identity matches win, recorded closed MRs are adoptable, exact-head matches -// beat looser pools, and only a unique candidate is ever selected. Fork MRs -// (source_project_id !== target_project_id) never identity-match, matching the -// GitHub head-repo identity requirement. +// Same decision ladder as the retained GitHub route, except MRs whose +// target_branch is not the requested base are rejected before identity-match +// or exact-head bind. A same-source-branch MR targeting a different base is +// never reused. Identity matches win, recorded closed MRs are adoptable, +// exact-head matches beat looser pools, and only a unique candidate is ever +// selected. Fork MRs (source_project_id !== target_project_id) never +// identity-match, matching the GitHub head-repo identity requirement. function selectGitlabMr(rows, { branch, baseBranch, localHeadSha = null, recordedCrNumber = null }) { const rowsList = Array.isArray(rows) ? rows : []; + const requestedTarget = rowsList.filter((row) => row?.target_branch === baseBranch); const sameHead = (row) => row?.source_branch === branch && sameGitlabProjectMr(row); - const identityMatches = rowsList.filter((row) => sameHead(row) && row.target_branch === baseBranch); - const pool = identityMatches.length ? identityMatches : rowsList.filter(sameHead); + const identityMatches = requestedTarget.filter(sameHead); + const pool = identityMatches; const byState = (state) => pool.filter((row) => row.state === state); const open = byState("opened"); const merged = byState("merged"); diff --git a/tests/relay-dispatch/scripts/forge-adapters.test.js b/tests/relay-dispatch/scripts/forge-adapters.test.js index 7d18a140..b76dfb48 100644 --- a/tests/relay-dispatch/scripts/forge-adapters.test.js +++ b/tests/relay-dispatch/scripts/forge-adapters.test.js @@ -77,6 +77,21 @@ function gitlabMr(overrides = {}) { }; } +function assertGitlabNoMatchCreatePath(observation) { + assert.equal(observation.available, true); + assert.equal(observation.lookup_complete, true); + assert.equal(observation.mr_lookup_complete, true); + assert.equal(observation.matching_mr_count, 0); + assert.equal(observation.identity_match_count, 0); + assert.equal(observation.mr_number, null); + assert.equal(observation.mr_state, null); + assert.equal(observation.head_ref, "issue-1210"); + assert.equal(observation.base_ref, "main"); + assert.equal(observation.mr_head_sha, null); + assert.equal(observation.error, undefined); + assert.equal(gitlabChangeRequestAdapter.exactPublishedChangeRequest(observation, { branch: "issue-1210" }, HEAD), false); +} + function recordingTransport(responses) { const calls = []; return { @@ -302,6 +317,87 @@ test("#1210 selects a unique candidate and fails ambiguity closed like the GitHu assert.equal(mergedObservation.merged_mr_count, 1); }); +test("#1339 rejects GitLab MRs whose target is not the requested base before identity-match or exact-head bind", async () => { + // Same source branch with only a different target is not reused, including + // when the live head already matches. Lookup stays complete so a new MR + // can still be created. + const wrongTargetOnly = recordingTransport([jsonResponse(200, [ + gitlabMr({ iid: 4, target_branch: "release" }), + ])]); + const wrongTargetObservation = await gitlabChangeRequestAdapter.observeChangeRequest({ + project: "group/repo", branch: "issue-1210", baseBranch: "main", + localHeadSha: HEAD, transport: wrongTargetOnly.transport, token: "t", + }); + assertGitlabNoMatchCreatePath(wrongTargetObservation); + assert.equal(wrongTargetObservation.open_mr_count, 0); + + // Among multiple targets, only the requested target can be selected. + const mixedTargets = recordingTransport([jsonResponse(200, [ + gitlabMr({ iid: 1, target_branch: "release" }), + gitlabMr({ iid: 2 }), + gitlabMr({ iid: 3, target_branch: "develop" }), + ])]); + const mixedObservation = await gitlabChangeRequestAdapter.observeChangeRequest({ + project: "group/repo", branch: "issue-1210", baseBranch: "main", + localHeadSha: HEAD, transport: mixedTargets.transport, token: "t", + }); + assert.equal(mixedObservation.mr_number, 2); + assert.equal(mixedObservation.base_ref, "main"); + assert.equal(mixedObservation.identity_match_count, 1); + assert.equal(mixedObservation.matching_mr_count, 1); + assert.equal(gitlabChangeRequestAdapter.exactPublishedChangeRequest(mixedObservation, { branch: "issue-1210" }, HEAD), true); + + // Non-requested targets never become an ambiguous leftover pool. + const onlyOtherTargets = recordingTransport([jsonResponse(200, [ + gitlabMr({ iid: 1, target_branch: "release" }), + gitlabMr({ iid: 3, target_branch: "develop" }), + ])]); + const onlyOtherObservation = await gitlabChangeRequestAdapter.observeChangeRequest({ + project: "group/repo", branch: "issue-1210", baseBranch: "main", + transport: onlyOtherTargets.transport, token: "t", + }); + assertGitlabNoMatchCreatePath(onlyOtherObservation); + + // A matching target still needs repository/source identity: a fork onto the + // requested base plus a same-project MR onto a different base is not reused. + const forkPlusWrongTarget = recordingTransport([jsonResponse(200, [ + gitlabMr({ iid: 8, source_project_id: 99, target_project_id: 7 }), + gitlabMr({ iid: 9, target_branch: "release" }), + ])]); + const forkPlusWrongObservation = await gitlabChangeRequestAdapter.observeChangeRequest({ + project: "group/repo", branch: "issue-1210", baseBranch: "main", + localHeadSha: HEAD, transport: forkPlusWrongTarget.transport, token: "t", + }); + assertGitlabNoMatchCreatePath(forkPlusWrongObservation); + assert.equal(forkPlusWrongObservation.fork_mr_count, 1); + + // Exact-head bind never reaches a wrong-target MR. A stale matching-target + // head is observed but does not bind, and does not lose to a different-target + // MR whose live head already matches. + const staleMatching = recordingTransport([jsonResponse(200, [ + gitlabMr({ iid: 1, sha: OTHER, diff_refs: { head_sha: OTHER, base_sha: BASE } }), + gitlabMr({ iid: 2, target_branch: "release" }), + ])]); + const staleObservation = await gitlabChangeRequestAdapter.observeChangeRequest({ + project: "group/repo", branch: "issue-1210", baseBranch: "main", + localHeadSha: HEAD, transport: staleMatching.transport, token: "t", + }); + assert.equal(staleObservation.mr_number, 1); + assert.equal(staleObservation.base_ref, "main"); + assert.equal(staleObservation.mr_head_sha, OTHER); + assert.equal(staleObservation.matching_mr_count, 1); + assert.equal(gitlabChangeRequestAdapter.exactPublishedChangeRequest(staleObservation, { branch: "issue-1210" }, HEAD), false); + + // Empty lookup is the documented no-match/create path; rejecting a wrong + // target must not look like an observation failure. + const empty = recordingTransport([jsonResponse(200, [])]); + const emptyObservation = await gitlabChangeRequestAdapter.observeChangeRequest({ + project: "group/repo", branch: "issue-1210", baseBranch: "main", + transport: empty.transport, token: "t", + }); + assertGitlabNoMatchCreatePath(emptyObservation); +}); + test("#1210 GitLab outages and permission failures stay typed, observable, and retry-safe", async () => { const cases = [ [jsonResponse(401, { message: "401 Unauthorized" }), "GITLAB_AUTH_INVALID", false], @@ -496,6 +592,49 @@ test("#1210 GitHub selection reuses the retained ladder for ambiguous and fork h assert.equal(recordedObservation.pr_state, "CLOSED"); }); +test("#1339 GitHub retained route still identity-matches the requested base and fails closed on a stale head", async (t) => { + const dir = tmpDir("forge-adapters-gh-1339-"); + t.after(() => fs.rmSync(dir, { recursive: true, force: true })); + + // Identity match (requested base) still wins over a looser same-head PR. + const identityWins = writeFakeGh(dir, { + rows: [githubRow({ number: 1, baseRefName: "release" }), githubRow({ number: 2 })], + }); + const identityObservation = await withEnv({ RELAY_GH_BIN: identityWins }, () => githubChangeRequestAdapter.observeChangeRequest({ + project: "owner/repo", branch: "issue-1210", baseBranch: "main", localHeadSha: HEAD, + })); + assert.equal(identityObservation.pr_number, 2); + assert.equal(identityObservation.base_ref, "main"); + assert.equal(identityObservation.identity_match_count, 1); + assert.equal(identityObservation.matching_pr_count, 1); + assert.equal(githubChangeRequestAdapter.exactPublishedChangeRequest(identityObservation, { branch: "issue-1210" }, HEAD), true); + + // A lagging live head is still observed (unique open candidate) but never binds. + const stale = writeFakeGh(dir, { + rows: [githubRow({ headRefOid: OTHER })], + }); + const staleObservation = await withEnv({ RELAY_GH_BIN: stale }, () => githubChangeRequestAdapter.observeChangeRequest({ + project: "owner/repo", branch: "issue-1210", baseBranch: "main", localHeadSha: HEAD, + })); + assert.equal(staleObservation.pr_number, 42); + assert.equal(staleObservation.pr_head_sha, OTHER); + assert.equal(staleObservation.matching_pr_count, 1); + assert.equal(githubChangeRequestAdapter.exactPublishedChangeRequest(staleObservation, { branch: "issue-1210" }, HEAD), false); + + // The retained GitHub ladder is unchanged: a unique same-head PR targeting a + // different base is still adoptable when no identity match exists. + const adopted = writeFakeGh(dir, { + rows: [githubRow({ number: 88, baseRefName: "main" })], + }); + const adoptedObservation = await withEnv({ RELAY_GH_BIN: adopted }, () => githubChangeRequestAdapter.observeChangeRequest({ + project: "owner/repo", branch: "issue-1210", baseBranch: "deleted-docs", localHeadSha: HEAD, + })); + assert.equal(adoptedObservation.pr_number, 88); + assert.equal(adoptedObservation.base_ref, "main"); + assert.equal(adoptedObservation.identity_match_count, 0); + assert.equal(adoptedObservation.matching_pr_count, 1); +}); + test("#1210 GitHub observation failures stay typed and retry-safe", async (t) => { const dir = tmpDir("forge-adapters-gh-fail-"); t.after(() => fs.rmSync(dir, { recursive: true, force: true }));