feat(mr): support viewing unresolved discussions on merge requests#98
feat(mr): support viewing unresolved discussions on merge requests#98haifeiWu wants to merge 1 commit into
Conversation
- Extend GitLabNoteSchema with resolved / resolved_by / resolved_at so the resolution metadata GitLab returns for MR discussion notes is no longer silently dropped at the Zod boundary. - Surface per-note and aggregated thread resolution status in formatDiscussionsResponse, and add an unresolved/resolved thread count to its summary line. - Add unresolved_only filter to list_merge_request_discussions so callers can fetch only threads with at least one unresolved resolvable note; filtering is done client-side since the GitLab discussions API has no server-side equivalent. - Cover the new schema + formatter behavior with vitest cases (96 tests total, +10 new).
There was a problem hiding this comment.
Code Review
This pull request introduces several enhancements to the GitLab MCP server, primarily focusing on merge request discussions. Key changes include the addition of an unresolved_only filter for the list_merge_request_discussions tool, the inclusion of resolution metadata (such as resolved, resolved_by, and resolved_at) in discussion notes, and improved summary reporting for resolved versus unresolved threads. Feedback was provided regarding inconsistencies in how unresolved threads are identified; specifically, the logic should consistently treat any resolvable note that is not explicitly true as unresolved to ensure the summary statistics align with the item-level flags and the client-side filtering logic.
| const resolvableThreads = discussions.items.filter(d => | ||
| d.notes.some(n => n.resolvable) | ||
| ); | ||
| const unresolvedCount = resolvableThreads.filter(d => | ||
| d.notes.some(n => n.resolvable && n.resolved === false) | ||
| ).length; | ||
| const resolvedCount = resolvableThreads.length - unresolvedCount; | ||
| const summary = resolvableThreads.length > 0 | ||
| ? `Found ${discussions.count} discussions (${unresolvedCount} unresolved, ${resolvedCount} resolved threads)` | ||
| : `Found ${discussions.count} discussions`; |
There was a problem hiding this comment.
The logic for calculating unresolvedCount is inconsistent with how the discussion-level resolved flag is computed later in the function (lines 347-349). Specifically, unresolvedCount uses n.resolved === false, while the discussion-level resolved flag uses n.resolved === true in an every check. If a resolvable note has resolved: undefined (which is allowed by the schema), the thread will be marked as resolved: false in the output but counted as "resolved" in the summary stats. Using a consistent check ensures the summary counts match the item flags.
| const resolvableThreads = discussions.items.filter(d => | |
| d.notes.some(n => n.resolvable) | |
| ); | |
| const unresolvedCount = resolvableThreads.filter(d => | |
| d.notes.some(n => n.resolvable && n.resolved === false) | |
| ).length; | |
| const resolvedCount = resolvableThreads.length - unresolvedCount; | |
| const summary = resolvableThreads.length > 0 | |
| ? `Found ${discussions.count} discussions (${unresolvedCount} unresolved, ${resolvedCount} resolved threads)` | |
| : `Found ${discussions.count} discussions`; | |
| const resolvableThreads = discussions.items.filter(d => | |
| d.notes.some(n => n.resolvable) | |
| ); | |
| const unresolvedCount = resolvableThreads.filter(d => | |
| d.notes.some(n => n.resolvable && n.resolved !== true) | |
| ).length; | |
| const resolvedCount = resolvableThreads.length - unresolvedCount; | |
| const summary = resolvableThreads.length > 0 | |
| ? `Found ${discussions.count} discussions (${unresolvedCount} unresolved, ${resolvedCount} resolved threads)` | |
| : `Found ${discussions.count} discussions`; |
| const filteredItems = discussions.items.filter(d => | ||
| d.notes.some(n => n.resolvable && n.resolved === false) | ||
| ); |
There was a problem hiding this comment.
The filtering logic here should be consistent with the definition of a resolved thread used in the formatter. A thread is unresolved if at least one of its resolvable notes is not resolved. Using n.resolved === false might miss threads where the resolved field is missing or null on a resolvable note, even though the formatter would treat such a thread as unresolved.
const filteredItems = discussions.items.filter(d =>
d.notes.some(n => n.resolvable && n.resolved !== true)
);
Summary
list_merge_request_discussionspreviously dropped GitLab's resolution metadata(
resolved,resolved_by,resolved_at) at the Zod boundary, leaving callerswith no way to tell which MR review threads were still open. This PR threads
that data all the way through — schema → formatter → tool output — and adds an
unresolved_onlyfilter so callers can fetch just the threads still needingattention. Fully additive: existing responses gain new optional fields, none
are removed or renamed.
Closes #
Type of change
Pre-merge checklist
package.jsonversion bumped — 0.8.1 → 0.9.0 (MINOR, new functionality)CHANGELOG.mdentry added under## [Unreleased]npm testpasses (vitest — 96/96, +10 new)npm run buildpassesfeat(mr):andchore(release):.envvalues in the difffeature/*,fix/*,docs/*, orrefactor/*(committed directly onmainin this fork; will need a feature branch if PR'd back upstream)src/index.ts—list_merge_request_discussionsretainsreadOnly: true; no impact on session lifecycle, token handling, or read-only modeFor breaking changes
Not applicable — fully additive change. The new fields on note objects and the
new optional argument on the tool are backwards-compatible with existing callers.
Notes for reviewer
What changed, file by file
src/schemas.ts—GitLabNoteSchemagainsresolved/resolved_by/resolved_at(all optional, withresolved_byreusingGitLabUserSchemaand nullable).ListMergeRequestDiscussionsSchemagainsoptional
unresolved_only: boolean.src/formatters.ts—formatDiscussionsResponsenow emits per-note resolution fields, computes a thread-levelresolvable/resolvedaggregate, and appends(N unresolved, M resolved threads)to the summaryline when any resolvable thread exists.
src/index.ts— handler forlist_merge_request_discussionsfilters discussions bynotes.some(n => n.resolvable && n.resolved === false)whenunresolved_only=true. Tool description updated.src/schemas.test.ts/src/formatters.test.ts— 10 new vitest cases.Design choices to look at
unresolvedfilter, so the handler fetches the requested page(s) first, then filters. This mirrors every other list tool in the server.For MRs with hundreds of discussions and aggressive pagination, callers should be aware they pay the full fetch cost.
resolvedsemantics. A thread is reported as resolved only when all its resolvable notes are resolved (notes.every(n => !n.resolvable || n.resolved === true)), matching the GitLab UI definition.Mixed threads (some resolved, some not) report
resolved=false.formatDiscussionsResponsefalls through to the legacy short summary and the new fields surface asundefinedforlist_issue_discussions— no behavioral change for that tool.Test coverage added
GitLabNoteSchema: unresolved resolvable note, resolved note with resolver metadata, note where the resolution fields are omitted entirely (issue-note path).ListMergeRequestDiscussionsSchema: required fields, acceptsunresolved_only: boolean, rejects non-boolean.formatDiscussionsResponse: per-note resolution fields pass through; multi-note thread with one unresolved note aggregates toresolved=false; summary count line for mixed threads; individual-note path keepsthe legacy short summary and emits no
resolvedaggregate.