Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
17 changes: 10 additions & 7 deletions skills/relay-dispatch/scripts/forge-adapters.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Filter by target branch before the 100-row API cap

When a source branch has more than 100 historical MRs across different targets, the request remains capped at per_page=100 without a target_branch filter, so this client-side filter can miss a valid MR for baseBranch on a later page. The adapter then incorrectly reports a complete empty lookup (lookup_complete: true, matching_mr_count: 0), allowing the consumer to attempt creation even though the matching MR already exists; include the target in the API query or paginate before declaring the lookup complete.

Useful? React with 👍 / 👎.

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");
Expand Down
139 changes: 139 additions & 0 deletions tests/relay-dispatch/scripts/forge-adapters.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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],
Expand Down Expand Up @@ -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 }));
Expand Down
Loading