feat: add list_group_members tool for member search - #633
Conversation
Closes #631 by exposing GitLab group member listing with query, inheritance, and pagination support alongside list_project_members. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesGroup member listing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ToolHandler
participant listGroupMembers
participant GitLabAPI
ToolHandler->>listGroupMembers: validated group-member options
listGroupMembers->>GitLabAPI: request direct or inherited members with filters
GitLabAPI-->>listGroupMembers: member response or error
listGroupMembers-->>ToolHandler: serialized member results
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 9-11: Consolidate the changelog entry by removing the duplicate
`##### Added` heading near the top and moving the `list_group_members` bullet
under the existing `##### Added` section.
In `@schemas.ts`:
- Around line 3391-3405: Update ListGroupMembersSchema to reject requests that
set both include_inheritance: true and skip_users, while preserving valid
direct-member and inherited-member requests. Add schema coverage for the invalid
combination, specifically include_inheritance: true with skip_users.
In `@test/test-list-group-members.ts`:
- Around line 158-169: Extend the “should pass query to the members endpoint”
test around callListGroupMembers to supply page and per_page alongside query,
capture those request parameters in the existing mock, and assert both values
are forwarded. Preserve the existing query assertion.
- Line 67: Update the catch clause in the relevant test parse-error handling
block to omit the unused binding, changing it to a parameterless catch while
preserving the existing catch body.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a41b08ac-43b2-45f3-bdda-3dbd81ff4ce3
📒 Files selected for processing (10)
CHANGELOG.mdREADME.mddocs/tools/index.mddocs/tools/projects.mdindex.tsschemas.tsskills/gitlab-mcp/SKILL.mdtest/test-list-group-members.tstest/test-toolset-filtering.tstools/registry.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: test
🧰 Additional context used
🪛 ast-grep (0.45.0)
test/test-list-group-members.ts
[warning] 2-2: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from 'child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🪛 ESLint
test/test-list-group-members.ts
[error] 67-67: 'e' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 LanguageTool
README.md
[grammar] ~736-~736: Ensure spelling is correct
Context: ...Add an emoji reaction to a work item (e.g. thumbsup, rocket, eyes) 188. `delete_work_item_e...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (8)
index.ts (1)
452-453: LGTM!Also applies to: 10941-10949
tools/registry.ts (1)
170-170: LGTM!Also applies to: 686-690, 1407-1407, 1760-1760
README.md (1)
638-638: LGTM!Also applies to: 639-766
docs/tools/index.md (1)
52-60: LGTM!docs/tools/projects.md (1)
11-11: LGTM!Also applies to: 111-128
skills/gitlab-mcp/SKILL.md (2)
61-61: LGTM!
8-20: 📐 Maintainability & Code QualityNo count change needed.
215 tools across 20 toolsets + execute_graphql + discover_toolsmatches the 217 total, anddocs/tools/page counts agree withdependency_proxy(4) andlabels(5).> Likely an incorrect or invalid review comment.test/test-toolset-filtering.ts (1)
37-37: LGTM!
| ##### Added | ||
|
|
||
| - Add `list_group_members` tool for searching group members by name or username ([#631](https://github.com/zereight/gitlab-mcp/issues/631)) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use one Unreleased Added section.
CHANGELOG.md already has an ##### Added heading at Line 21. Move this bullet under that heading and remove the duplicate heading at Lines 9-10.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` around lines 9 - 11, Consolidate the changelog entry by
removing the duplicate `##### Added` heading near the top and moving the
`list_group_members` bullet under the existing `##### Added` section.
| export const ListGroupMembersSchema = z.object({ | ||
| group_id: z.string().describe("Group ID or URL-encoded path"), | ||
| query: z.string().optional().describe("Search for members by name or username"), | ||
| user_ids: z.array(z.coerce.number()).optional().describe("Filter by user IDs"), | ||
| skip_users: z.array(z.coerce.number()).optional().describe("User IDs to exclude"), | ||
| include_inheritance: z.coerce | ||
| .boolean() | ||
| .optional() | ||
| .describe("Include inherited members. Defaults to false."), | ||
| per_page: z.coerce | ||
| .number() | ||
| .optional() | ||
| .describe("Number of items per page (default: 20, max: 100)"), | ||
| page: z.coerce.number().optional().describe("Page number for pagination (default: 1)"), | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)schemas\.ts$|(^|/)index\.ts$' || true
echo "== relevant schema occurrences =="
rg -n "ListGroupMembersSchema|listGroupMembers|skip_users|include_inheritance|modes?/all|members/all" -S .
echo "== schemas context =="
sed -n '3360,3420p' schemas.ts || true
echo "== implementation context =="
rg -n "ListGroupMembersSchema|listGroupMembers|members/all|members\\?|skip_users" -S --glob '*.ts' .Repository: zereight/gitlab-mcp
Length of output: 6648
🌐 Web query:
GitLab API list group members skip_users members/all include_inheritance
💡 Result:
In the GitLab API, the behavior regarding membership inheritance is determined by the specific endpoint chosen rather than a dedicated parameter like "include_inheritance" [1][2]. To list group members, you use two primary endpoints: 1. List direct members: GET /groups/:id/members This endpoint returns only members directly assigned to the specified group [1][3]. It does not include members inherited from ancestor groups or invited groups [1][2]. This endpoint supports the skip_users parameter, which allows you to filter out specific user IDs from the results [1][2]. 2. List all members (including inherited): GET /groups/:id/members/all This endpoint returns all members of the specified group, including those inherited from ancestor groups and invited users [1][2]. When a user is a member of both the group and an ancestor group, this endpoint returns the membership with the highest access level, representing the user's effective permission [4]. There is no "include_inheritance" parameter; inheritance is implicit when using the /members/all endpoint [1][2]. The skip_users parameter is available for the direct members endpoint (/members) but is not documented as a parameter for the /members/all endpoint [1][2].
Citations:
- 1: https://docs.gitlab.com/api/group_members/
- 2: https://docs.gitlab.com/17.9/api/members/
- 3: https://docs.gitlab.com/18.6/api/group_members/
- 4: https://gitlab.com/gitlab-org/gitlab/-/blob/1e718b1777d9/doc/api/members.md
Disallow skip_users for inherited group members.
listGroupMembers forwards skip_users[] unconditionally, so include_inheritance: true sends it to /groups/:id/members/all. skip_users is documented for the direct /groups/:id/members endpoint only; reject this combination in ListGroupMembersSchema and add a test for include_inheritance: true with skip_users.
Proposed fix
export const ListGroupMembersSchema = z.object({
group_id: z.string().describe("Group ID or URL-encoded path"),
query: z.string().optional().describe("Search for members by name or username"),
user_ids: z.array(z.coerce.number()).optional().describe("Filter by user IDs"),
skip_users: z.array(z.coerce.number()).optional().describe("User IDs to exclude"),
include_inheritance: z.coerce.boolean().optional(),
per_page: z.coerce.number().optional(),
page: z.coerce.number().optional(),
+}).superRefine((value, ctx) => {
+ if (value.include_inheritance && value.skip_users?.length) {
+ ctx.addIssue({
+ code: z.ZodIssueCode.custom,
+ path: ["skip_users"],
+ message: "skip_users is not supported when include_inheritance is true",
+ });
+ }
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@schemas.ts` around lines 3391 - 3405, Update ListGroupMembersSchema to reject
requests that set both include_inheritance: true and skip_users, while
preserving valid direct-member and inherited-member requests. Add schema
coverage for the invalid combination, specifically include_inheritance: true
with skip_users.
| if (content) { | ||
| try { | ||
| resolve(JSON.parse(content)); | ||
| } catch (e) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the unused catch binding.
ESLint reports that e is unused. This can fail the lint check. Use catch { ... } because this branch does not use the parse error.
Proposed fix
- } catch (e) {
+ } catch {
reject(new Error(`Failed to parse tool output JSON: ${content}`));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (e) { | |
| } catch { |
🧰 Tools
🪛 ESLint
[error] 67-67: 'e' is defined but never used.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/test-list-group-members.ts` at line 67, Update the catch clause in the
relevant test parse-error handling block to omit the unused binding, changing it
to a parameterless catch while preserving the existing catch body.
Source: Linters/SAST tools
| describe('with query', () => { | ||
| test('should pass query to the members endpoint', async () => { | ||
| await callListGroupMembers( | ||
| { group_id: TEST_GROUP_ID, query: 'alice' }, | ||
| { | ||
| GITLAB_API_URL: `${mockGitLabUrl}/api/v4`, | ||
| GITLAB_PERSONAL_ACCESS_TOKEN: MOCK_TOKEN | ||
| } | ||
| ); | ||
|
|
||
| assert.strictEqual(queryParam, 'alice', 'Query parameter should be forwarded'); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add pagination forwarding coverage.
The tool contract supports pagination, but this test only records query. Record and assert page and per_page for a request that supplies both options. This prevents regressions where the handler drops pagination parameters.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/test-list-group-members.ts` around lines 158 - 169, Extend the “should
pass query to the members endpoint” test around callListGroupMembers to supply
page and per_page alongside query, capture those request parameters in the
existing mock, and assert both values are forwarded. Preserve the existing query
assertion.
1 participant
Summary
list_group_membersMCP tool to search and list GitLab group members by name or username (query), with optional inherited members and paginationlist_project_membersagainstGET /groups/:id/membersTest plan
node --import tsx/esm --test test/test-list-group-members.tsnpm run check:skill-syncnode --import tsx/esm --test test/test-toolset-filtering.tsMade with Cursor