From c2a89acc04d0324cc08f5d97f3bfc6a51f6a9fe4 Mon Sep 17 00:00:00 2001 From: "sergey.kalin" Date: Wed, 5 Aug 2026 15:03:54 +0300 Subject: [PATCH] fix: detect approvals when approval_state reports no rules get_merge_request_approval_state derives approved_by solely from approval_state's rules array, and only falls back to /approvals on a 404. Approval rules are a paid-tier feature. A project without them answers GET /approval_state with HTTP 200 and `rules: []`, so the flatMap over rules yields no approvers even when someone has approved the MR. The fallback never fires because the request succeeded. Callers are told nobody approved. The same response also leaves `approved`, `user_has_approved` and `user_can_approve` null, since approval_state does not carry them -- only /approvals does. Two changes: - fall back on 402/403 as well as 404, for instances or namespaces that reject the endpoint rather than 404 it; - when approval_state returns an empty rules array, read /approvals and prefer its result if it lists approvers. Wrapped in try/catch so an unavailable /approvals keeps the previous behaviour. Keyed on the rules array rather than on the approver count, so a project using approval rules makes no extra request even while an MR is still waiting for its first approval -- a non-empty rules array means rules are in use and approval_state is authoritative. The added test covers the empty-rules case; the existing rules test asserts that path still resolves from approval_state alone. Co-Authored-By: Claude Opus 5 --- index.ts | 24 +++++++- ...test-merge-request-approval-state-tools.ts | 58 +++++++++++++++++++ 2 files changed, 81 insertions(+), 1 deletion(-) diff --git a/index.ts b/index.ts index 0319f35bd..c0c4b7410 100644 --- a/index.ts +++ b/index.ts @@ -5806,7 +5806,13 @@ async function getMergeRequestApprovalState( method: "GET", }); - if (approvalStateResponse.status === 404) { + // 404 when the endpoint is unavailable; 402/403 when the instance or namespace is + // not licensed for approval rules, which are a paid-tier feature. + if ( + approvalStateResponse.status === 404 || + approvalStateResponse.status === 402 || + approvalStateResponse.status === 403 + ) { return getMergeRequestApprovalsFallback(projectId, mergeRequestIid); } @@ -5820,6 +5826,22 @@ async function getMergeRequestApprovalState( ); const approvedByUsernames = approvedByUsers.map(user => user.username); + // approval_state only reports approvals attributed to rules, so a project with no + // approval rules answers 200 with `rules: []` and yields no approvers even when + // someone has approved. Read /approvals in that case. A non-empty rules array means + // rules are in use and approval_state is authoritative, so no extra request is made + // there - including while an MR is still waiting for its first approval. + if ((parsedApprovalState.rules || []).length === 0) { + try { + const viaApprovals = await getMergeRequestApprovalsFallback(projectId, mergeRequestIid); + if ((viaApprovals.approved_by ?? []).length > 0) { + return viaApprovals; + } + } catch { + // /approvals unavailable as well - keep the approval_state answer below. + } + } + return { ...parsedApprovalState, approved_by: approvedByUsers, diff --git a/test/test-merge-request-approval-state-tools.ts b/test/test-merge-request-approval-state-tools.ts index 07f19eb51..e5cc018cf 100644 --- a/test/test-merge-request-approval-state-tools.ts +++ b/test/test-merge-request-approval-state-tools.ts @@ -7,6 +7,7 @@ const MOCK_TOKEN = "glpat-mock-token-approval"; const TEST_PROJECT_ID = "123"; const TEST_MR_IID_WITH_FALLBACK = "88"; const TEST_MR_IID_WITH_APPROVAL_STATE = "89"; +const TEST_MR_IID_WITHOUT_RULES = "90"; async function callTool( toolName: string, @@ -160,6 +161,43 @@ describe("merge request approval state tools", () => { } ); + // A project without approval rules answers 200 with an empty rules array, while + // the approval itself is only visible on /approvals. + mockGitLab.addMockHandler( + "get", + `/projects/${TEST_PROJECT_ID}/merge_requests/${TEST_MR_IID_WITHOUT_RULES}/approval_state`, + (_req, res) => { + res.json({ + approval_rules_overwritten: false, + rules: [], + }); + } + ); + + mockGitLab.addMockHandler( + "get", + `/projects/${TEST_PROJECT_ID}/merge_requests/${TEST_MR_IID_WITHOUT_RULES}/approvals`, + (_req, res) => { + res.json({ + approved: true, + user_has_approved: false, + user_can_approve: true, + approved_by: [ + { + user: { + id: "35", + username: "sergey.kravchenya", + name: "Sergey Kravchenya", + state: "active", + avatar_url: "https://gitlab.mock/uploads/avatar.png", + web_url: "https://gitlab.mock/sergey.kravchenya", + }, + }, + ], + }); + } + ); + await mockGitLab.start(); mockGitLabUrl = mockGitLab.getUrl(); }); @@ -188,6 +226,26 @@ describe("merge request approval state tools", () => { assert.strictEqual(result.approved_by[0].username, "sergey.kravchenya"); }); + test("finds approvers via approvals when approval_state reports no rules", async () => { + const result = await callTool( + "get_merge_request_approval_state", + { + project_id: TEST_PROJECT_ID, + merge_request_iid: TEST_MR_IID_WITHOUT_RULES, + }, + { + GITLAB_API_URL: `${mockGitLabUrl}/api/v4`, + GITLAB_PERSONAL_ACCESS_TOKEN: MOCK_TOKEN, + } + ); + + assert.strictEqual(result.source_endpoint, "approvals"); + assert.strictEqual(result.approved, true); + assert.deepStrictEqual(result.approved_by_usernames, ["sergey.kravchenya"]); + assert.ok(Array.isArray(result.approved_by)); + assert.strictEqual(result.approved_by[0].username, "sergey.kravchenya"); + }); + test("returns deduplicated approvers from approval_state rules", async () => { const result = await callTool( "get_merge_request_approval_state",