diff --git a/index.ts b/index.ts index 0319f35b..c0c4b741 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 07f19eb5..e5cc018c 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",