diff --git a/skill-data/drupalorg-cli/SKILL.md b/skill-data/drupalorg-cli/SKILL.md index c86a1c15..029ff5ae 100644 --- a/skill-data/drupalorg-cli/SKILL.md +++ b/skill-data/drupalorg-cli/SKILL.md @@ -107,8 +107,9 @@ includes the MR IID (`!iid`), the second `` argument is not needed. # List merge requests for a Drupal.org issue fork # --state: opened (default), closed, merged, all # nid is optional; auto-detected from the branch name if omitted +# Only MRs opened from the issue fork are returned; empty means no fork or no MRs drupalorg mr:list [nid] [--state=opened] --format=llm -# List MRs by project path (no issue NID needed) +# List every MR on a project (not scoped to an issue) drupalorg mr:list project/drupal --format=llm # Show the unified diff for a merge request diff --git a/skill-data/drupalorg-cli/references/gitlab-mr-contribution.md b/skill-data/drupalorg-cli/references/gitlab-mr-contribution.md index c6d15206..b9b9eb72 100644 --- a/skill-data/drupalorg-cli/references/gitlab-mr-contribution.md +++ b/skill-data/drupalorg-cli/references/gitlab-mr-contribution.md @@ -73,6 +73,12 @@ drupalorg mr:list --state=merged --format=llm drupalorg mr:list --state=all --format=llm ``` +`mr:list ` only returns MRs opened from that issue's fork +(`issue/{project}-{nid}`). The `issue_fork` field names the fork the list was +scoped to. An empty list means the issue has no fork or no MRs in the requested +state, never that MRs live elsewhere. To list every MR on a project, pass the +project path instead: `mr:list project/drupal`. + `--format=llm` output includes IID, title, source branch, state, mergeability, author, and last-updated timestamp for each MR. diff --git a/skills/drupalorg-cli/references/gitlab-mr-contribution.md b/skills/drupalorg-cli/references/gitlab-mr-contribution.md index c6d15206..b9b9eb72 100644 --- a/skills/drupalorg-cli/references/gitlab-mr-contribution.md +++ b/skills/drupalorg-cli/references/gitlab-mr-contribution.md @@ -73,6 +73,12 @@ drupalorg mr:list --state=merged --format=llm drupalorg mr:list --state=all --format=llm ``` +`mr:list ` only returns MRs opened from that issue's fork +(`issue/{project}-{nid}`). The `issue_fork` field names the fork the list was +scoped to. An empty list means the issue has no fork or no MRs in the requested +state, never that MRs live elsewhere. To list every MR on a project, pass the +project path instead: `mr:list project/drupal`. + `--format=llm` output includes IID, title, source branch, state, mergeability, author, and last-updated timestamp for each MR. diff --git a/src/Api/Action/MergeRequest/ListMergeRequestsAction.php b/src/Api/Action/MergeRequest/ListMergeRequestsAction.php index c9009639..f5257d91 100644 --- a/src/Api/Action/MergeRequest/ListMergeRequestsAction.php +++ b/src/Api/Action/MergeRequest/ListMergeRequestsAction.php @@ -2,6 +2,7 @@ namespace mglaman\DrupalOrg\Action\MergeRequest; +use GuzzleHttp\Exception\ClientException; use mglaman\DrupalOrg\Enum\MergeRequestState; use mglaman\DrupalOrg\GitLab\MergeRequestRef; use mglaman\DrupalOrg\Result\MergeRequest\MergeRequestItem; @@ -9,24 +10,82 @@ class ListMergeRequestsAction extends AbstractMergeRequestAction { - public function __invoke(string $nid, MergeRequestState $state = MergeRequestState::Opened, ?MergeRequestRef $ref = null): MergeRequestListResult - { - [$projectId, $gitLabProjectPath] = $ref !== null ? $this->resolveFromRef($ref) : $this->resolveGitLabProject($nid); - + /** + * Lists merge requests for an issue fork, or for a whole project. + * + * Merge requests belong to the target project on GitLab, so listing them + * on the fork returns nothing. Instead this lists the parent project and + * filters by the fork's project ID. A missing fork yields an empty list. + * + * @param string|null $projectMachineName + * Skips the Drupal.org node lookup when the caller already knows the + * project, such as from a WorkItemRef. + */ + public function __invoke( + string $nid, + MergeRequestState $state = MergeRequestState::Opened, + ?MergeRequestRef $ref = null, + ?string $projectMachineName = null, + ): MergeRequestListResult { $params = ['per_page' => 100]; if ($state !== MergeRequestState::All) { $params['state'] = $state->value; } - $mrObjects = $this->gitLabClient->getMergeRequests($projectId, $params); - $mergeRequests = array_map( - static fn(\stdClass $mr) => MergeRequestItem::fromStdClass($mr), - $mrObjects - ); + if ($ref !== null) { + [$projectId, $projectPath] = $this->resolveFromRef($ref); + return new MergeRequestListResult( + projectPath: $projectPath, + mergeRequests: $this->fetch($projectId, $params), + ); + } + + if ($projectMachineName === null) { + $projectMachineName = $this->client->getNode($nid)->fieldProjectMachineName; + } + $projectPath = 'project/' . $projectMachineName; + $issueForkPath = 'issue/' . $projectMachineName . '-' . $nid; + + $forkId = $this->findProjectId($issueForkPath); + if ($forkId === null) { + return new MergeRequestListResult( + projectPath: $projectPath, + mergeRequests: [], + issueFork: $issueForkPath, + ); + } + + $project = $this->gitLabClient->getProject($projectPath); + $params['source_project_id'] = $forkId; return new MergeRequestListResult( - projectPath: $gitLabProjectPath, - mergeRequests: $mergeRequests, + projectPath: $projectPath, + mergeRequests: $this->fetch((int) $project->id, $params), + issueFork: $issueForkPath, ); } + + /** + * @param array $params + * @return MergeRequestItem[] + */ + private function fetch(int $projectId, array $params): array + { + return array_map( + static fn(\stdClass $mr) => MergeRequestItem::fromStdClass($mr), + $this->gitLabClient->getMergeRequests($projectId, $params) + ); + } + + private function findProjectId(string $path): ?int + { + try { + return (int) $this->gitLabClient->getProject($path)->id; + } catch (ClientException $e) { + if ($e->getResponse()->getStatusCode() === 404) { + return null; + } + throw $e; + } + } } diff --git a/src/Api/Mcp/ToolRegistry.php b/src/Api/Mcp/ToolRegistry.php index 4dbab828..88d39522 100644 --- a/src/Api/Mcp/ToolRegistry.php +++ b/src/Api/Mcp/ToolRegistry.php @@ -153,7 +153,7 @@ public function maintainerGetIssues( return (new GetMaintainerIssuesAction())($user, MaintainerIssueType::from($type))->jsonSerialize(); } - #[McpTool(annotations: new ToolAnnotations(readOnlyHint: true, destructiveHint: false, idempotentHint: true, openWorldHint: true), name: 'mr_list', description: 'List merge requests for an issue fork.')] + #[McpTool(annotations: new ToolAnnotations(readOnlyHint: true, destructiveHint: false, idempotentHint: true, openWorldHint: true), name: 'mr_list', description: 'List merge requests opened from a Drupal.org issue fork. Returns an empty list when the issue has no fork.')] public function mrList( #[Schema(description: 'The Drupal.org issue node ID.', pattern: self::NID_PATTERN)] string $nid, diff --git a/src/Api/Result/MergeRequest/MergeRequestListResult.php b/src/Api/Result/MergeRequest/MergeRequestListResult.php index 3d7ce807..e77fc8d5 100644 --- a/src/Api/Result/MergeRequest/MergeRequestListResult.php +++ b/src/Api/Result/MergeRequest/MergeRequestListResult.php @@ -8,10 +8,14 @@ class MergeRequestListResult implements ResultInterface { /** * @param MergeRequestItem[] $mergeRequests + * @param string|null $issueFork + * The issue fork path the list is scoped to, or null for a + * project-wide list. */ public function __construct( public readonly string $projectPath, public readonly array $mergeRequests, + public readonly ?string $issueFork = null, ) { } @@ -19,6 +23,7 @@ public function jsonSerialize(): mixed { return [ 'project_path' => $this->projectPath, + 'issue_fork' => $this->issueFork, 'merge_requests' => array_map( static fn(MergeRequestItem $mr) => $mr->toArray(), $this->mergeRequests diff --git a/src/Cli/Command/MergeRequest/ListMergeRequests.php b/src/Cli/Command/MergeRequest/ListMergeRequests.php index 193acc8a..0e05d711 100644 --- a/src/Cli/Command/MergeRequest/ListMergeRequests.php +++ b/src/Cli/Command/MergeRequest/ListMergeRequests.php @@ -58,20 +58,31 @@ protected function initialize(InputInterface $input, OutputInterface $output): v parent::initialize($input, $output); } + private function projectMachineName(): ?string + { + if ($this->workItemRef === null) { + return null; + } + return substr($this->workItemRef->projectPath, strlen('project/')); + } + protected function execute(InputInterface $input, OutputInterface $output): int { $state = MergeRequestState::from((string) ($this->stdIn->getOption('state') ?? 'opened')); $format = (string) ($this->stdIn->getOption('format') ?? 'text'); $action = new ListMergeRequestsAction($this->client, new GitLabClient()); - $result = $action($this->nid ?? '', $state, $this->mrRef); + $result = $action($this->nid ?? '', $state, $this->mrRef, $this->projectMachineName()); if ($this->writeFormatted($result, $format)) { return 0; } if ($result->mergeRequests === []) { - $this->stdOut->writeln(sprintf('No %s merge requests found.', $state->value)); + $scope = $result->issueFork !== null + ? sprintf('for issue fork %s', $result->issueFork) + : sprintf('in %s', $result->projectPath); + $this->stdOut->writeln(sprintf('No %s merge requests found %s.', $state->value, $scope)); return 0; } diff --git a/src/Cli/Command/MergeRequest/MrCommandBase.php b/src/Cli/Command/MergeRequest/MrCommandBase.php index 406a15f6..325c280e 100644 --- a/src/Cli/Command/MergeRequest/MrCommandBase.php +++ b/src/Cli/Command/MergeRequest/MrCommandBase.php @@ -66,9 +66,12 @@ protected function initialize(InputInterface $input, OutputInterface $output): v return; } - // mr-iid not provided — auto-select from open merge requests. + // mr-iid not provided — auto-select from the issue fork's open merge requests. + $projectMachineName = $this->workItemRef !== null + ? substr($this->workItemRef->projectPath, strlen('project/')) + : null; $listAction = new ListMergeRequestsAction($this->client, new GitLabClient()); - $listResult = $listAction($this->nid, MergeRequestState::Opened); + $listResult = $listAction($this->nid, MergeRequestState::Opened, null, $projectMachineName); $mergeRequests = $listResult->mergeRequests; if ($mergeRequests === []) { diff --git a/src/Cli/Formatter/LlmFormatter.php b/src/Cli/Formatter/LlmFormatter.php index 8851dfe7..287c58b6 100644 --- a/src/Cli/Formatter/LlmFormatter.php +++ b/src/Cli/Formatter/LlmFormatter.php @@ -167,6 +167,9 @@ protected function formatIssueFork(IssueForkResult $result): string protected function formatMergeRequestList(MergeRequestListResult $result): string { $projectPath = $this->xmlEscape($result->projectPath); + $issueFork = $result->issueFork !== null + ? " " . $this->xmlEscape($result->issueFork) . "\n" + : ''; $items = ''; foreach ($result->mergeRequests as $mr) { $title = $this->xmlEscape($mr->title); @@ -188,7 +191,7 @@ protected function formatMergeRequestList(MergeRequestListResult $result): strin $items .= " {$updatedAt}\n"; $items .= " \n"; } - return "\n {$projectPath}\n \n{$items} \n"; + return "\n {$projectPath}\n{$issueFork} \n{$items} \n"; } protected function formatMergeRequestStatus(MergeRequestStatusResult $result): string diff --git a/src/Cli/Formatter/MarkdownFormatter.php b/src/Cli/Formatter/MarkdownFormatter.php index 1f57b1af..5e766b52 100644 --- a/src/Cli/Formatter/MarkdownFormatter.php +++ b/src/Cli/Formatter/MarkdownFormatter.php @@ -133,6 +133,10 @@ protected function formatMergeRequestList(MergeRequestListResult $result): strin $lines = []; $lines[] = "# Merge Requests: {$result->projectPath}"; $lines[] = ''; + if ($result->issueFork !== null) { + $lines[] = "Scoped to issue fork `{$result->issueFork}`."; + $lines[] = ''; + } foreach ($result->mergeRequests as $mr) { $mergeable = $mr->isMergeable ? ' ✓' : ''; $lines[] = "- **!{$mr->iid}** [{$mr->state}{$mergeable}] [{$mr->title}]({$mr->webUrl})"; diff --git a/tests/src/Action/MergeRequest/ListMergeRequestsActionTest.php b/tests/src/Action/MergeRequest/ListMergeRequestsActionTest.php index 85c54b3f..0abdb7fe 100644 --- a/tests/src/Action/MergeRequest/ListMergeRequestsActionTest.php +++ b/tests/src/Action/MergeRequest/ListMergeRequestsActionTest.php @@ -2,11 +2,15 @@ namespace mglaman\DrupalOrg\Tests\Action\MergeRequest; +use GuzzleHttp\Exception\ClientException; +use GuzzleHttp\Psr7\Request; +use GuzzleHttp\Psr7\Response; use mglaman\DrupalOrg\Action\MergeRequest\ListMergeRequestsAction; use mglaman\DrupalOrg\Client; use mglaman\DrupalOrg\Enum\MergeRequestState; use mglaman\DrupalOrg\Entity\IssueNode; use mglaman\DrupalOrg\GitLab\Client as GitLabClient; +use mglaman\DrupalOrg\GitLab\MergeRequestRef; use mglaman\DrupalOrg\Result\MergeRequest\MergeRequestItem; use mglaman\DrupalOrg\Result\MergeRequest\MergeRequestListResult; use PHPUnit\Framework\Attributes\CoversClass; @@ -17,6 +21,9 @@ #[CoversClass(MergeRequestItem::class)] class ListMergeRequestsActionTest extends TestCase { + private const PROJECT_ID = 12345; + private const FORK_ID = 67890; + private static function makeIssueNode(): IssueNode { return new IssueNode( @@ -39,10 +46,10 @@ private static function makeIssueNode(): IssueNode ); } - private static function makeProject(): \stdClass + private static function makeProject(int $id): \stdClass { $project = new \stdClass(); - $project->id = 12345; + $project->id = $id; return $project; } @@ -57,68 +64,138 @@ private static function makeMrObject(int $iid = 7, string $state = 'opened'): \s $mr->source_branch = '3383637-fix-the-bug'; $mr->target_branch = '11.x'; $mr->state = $state; - $mr->web_url = 'https://git.drupalcode.org/issue/drupal-3383637/-/merge_requests/' . $iid; + $mr->web_url = 'https://git.drupalcode.org/project/drupal/-/merge_requests/' . $iid; $mr->merge_status = 'can_be_merged'; $mr->author = $author; $mr->updated_at = '2024-01-15T10:00:00Z'; return $mr; } - public function testListWithStateFilter(): void + private static function notFound(): ClientException + { + return new ClientException('Not Found', new Request('GET', 'projects/issue%2Fdrupal-3383637'), new Response(404)); + } + + /** + * @return \PHPUnit\Framework\MockObject\MockObject&GitLabClient + */ + private function gitLabClientWithFork(): GitLabClient + { + $gitLabClient = $this->createMock(GitLabClient::class); + $gitLabClient->method('getProject')->willReturnMap([ + ['project/drupal', self::makeProject(self::PROJECT_ID)], + ['issue/drupal-3383637', self::makeProject(self::FORK_ID)], + ]); + return $gitLabClient; + } + + public function testListsOnlyMergeRequestsFromTheIssueFork(): void { $client = $this->createMock(Client::class); $client->method('getNode')->with('3383637')->willReturn(self::makeIssueNode()); - $gitLabClient = $this->createMock(GitLabClient::class); - $gitLabClient->method('getProject')->with('project/drupal')->willReturn(self::makeProject()); + $gitLabClient = $this->gitLabClientWithFork(); $gitLabClient->expects($this->once()) ->method('getMergeRequests') - ->with(12345, ['per_page' => 100, 'state' => 'opened']) + ->with(self::PROJECT_ID, ['per_page' => 100, 'state' => 'opened', 'source_project_id' => self::FORK_ID]) ->willReturn([self::makeMrObject()]); $action = new ListMergeRequestsAction($client, $gitLabClient); $result = $action('3383637', MergeRequestState::Opened); - self::assertInstanceOf(MergeRequestListResult::class, $result); self::assertSame('project/drupal', $result->projectPath); + self::assertSame('issue/drupal-3383637', $result->issueFork); self::assertCount(1, $result->mergeRequests); - self::assertInstanceOf(MergeRequestItem::class, $result->mergeRequests[0]); self::assertSame(7, $result->mergeRequests[0]->iid); self::assertSame('opened', $result->mergeRequests[0]->state); self::assertSame('mglaman', $result->mergeRequests[0]->author); self::assertTrue($result->mergeRequests[0]->isMergeable); + self::assertSame('issue/drupal-3383637', $result->jsonSerialize()['issue_fork']); } - public function testAllStateOmitsStateParam(): void + public function testMissingForkReturnsEmptyListInsteadOfProjectMergeRequests(): void { $client = $this->createMock(Client::class); $client->method('getNode')->willReturn(self::makeIssueNode()); $gitLabClient = $this->createMock(GitLabClient::class); - $gitLabClient->method('getProject')->willReturn(self::makeProject()); + $gitLabClient->method('getProject')->willThrowException(self::notFound()); + $gitLabClient->expects($this->never())->method('getMergeRequests'); + + $action = new ListMergeRequestsAction($client, $gitLabClient); + $result = $action('3383637', MergeRequestState::Opened); + + self::assertSame([], $result->mergeRequests); + self::assertSame('project/drupal', $result->projectPath); + self::assertSame('issue/drupal-3383637', $result->issueFork); + } + + public function testNonNotFoundGitLabErrorsPropagate(): void + { + $client = $this->createMock(Client::class); + $client->method('getNode')->willReturn(self::makeIssueNode()); + + $gitLabClient = $this->createMock(GitLabClient::class); + $gitLabClient->method('getProject')->willThrowException( + new ClientException('Forbidden', new Request('GET', 'projects/x'), new Response(403)) + ); + + $action = new ListMergeRequestsAction($client, $gitLabClient); + + $this->expectException(ClientException::class); + $action('3383637', MergeRequestState::Opened); + } + + public function testProjectMachineNameSkipsDrupalOrgLookup(): void + { + $client = $this->createMock(Client::class); + $client->expects($this->never())->method('getNode'); + + $gitLabClient = $this->gitLabClientWithFork(); + $gitLabClient->method('getMergeRequests')->willReturn([self::makeMrObject()]); + + $action = new ListMergeRequestsAction($client, $gitLabClient); + $result = $action('3383637', MergeRequestState::Opened, null, 'drupal'); + + self::assertSame('issue/drupal-3383637', $result->issueFork); + self::assertCount(1, $result->mergeRequests); + } + + public function testProjectRefListsWholeProject(): void + { + $client = $this->createMock(Client::class); + $client->expects($this->never())->method('getNode'); + + $gitLabClient = $this->createMock(GitLabClient::class); + $gitLabClient->method('getProject')->with('project/drupal')->willReturn(self::makeProject(self::PROJECT_ID)); $gitLabClient->expects($this->once()) ->method('getMergeRequests') - ->with(12345, ['per_page' => 100]) - ->willReturn([self::makeMrObject(7, 'opened'), self::makeMrObject(6, 'merged')]); + ->with(self::PROJECT_ID, ['per_page' => 100, 'state' => 'opened']) + ->willReturn([self::makeMrObject(7), self::makeMrObject(6)]); $action = new ListMergeRequestsAction($client, $gitLabClient); - $result = $action('3383637', MergeRequestState::All); + $result = $action('', MergeRequestState::Opened, new MergeRequestRef('project/drupal')); + self::assertSame('project/drupal', $result->projectPath); + self::assertNull($result->issueFork); self::assertCount(2, $result->mergeRequests); + self::assertNull($result->jsonSerialize()['issue_fork']); } - public function testEmptyResult(): void + public function testAllStateOmitsStateParam(): void { $client = $this->createMock(Client::class); $client->method('getNode')->willReturn(self::makeIssueNode()); - $gitLabClient = $this->createMock(GitLabClient::class); - $gitLabClient->method('getProject')->willReturn(self::makeProject()); - $gitLabClient->method('getMergeRequests')->willReturn([]); + $gitLabClient = $this->gitLabClientWithFork(); + $gitLabClient->expects($this->once()) + ->method('getMergeRequests') + ->with(self::PROJECT_ID, ['per_page' => 100, 'source_project_id' => self::FORK_ID]) + ->willReturn([self::makeMrObject(7, 'opened'), self::makeMrObject(6, 'merged')]); $action = new ListMergeRequestsAction($client, $gitLabClient); - $result = $action('3383637', MergeRequestState::Opened); + $result = $action('3383637', MergeRequestState::All); - self::assertSame([], $result->mergeRequests); + self::assertCount(2, $result->mergeRequests); } } diff --git a/tests/src/Formatter/LlmFormatterTest.php b/tests/src/Formatter/LlmFormatterTest.php index 252928f1..737912d6 100644 --- a/tests/src/Formatter/LlmFormatterTest.php +++ b/tests/src/Formatter/LlmFormatterTest.php @@ -257,15 +257,17 @@ public function testMergeRequestListResult(): void ); $result = new MergeRequestListResult( - projectPath: 'issue/drupal-3383637', + projectPath: 'project/drupal', mergeRequests: [$mr], + issueFork: 'issue/drupal-3383637', ); $formatter = new LlmFormatter(); $output = $formatter->format($result); self::assertStringContainsString('', $output); - self::assertStringContainsString('issue/drupal-3383637', $output); + self::assertStringContainsString('project/drupal', $output); + self::assertStringContainsString('issue/drupal-3383637', $output); self::assertStringContainsString('7', $output); self::assertStringContainsString('Fix <b>broken</b> & stuff', $output); self::assertStringContainsString('opened', $output); diff --git a/tests/src/Formatter/MarkdownFormatterTest.php b/tests/src/Formatter/MarkdownFormatterTest.php index 0731decb..e66c397a 100644 --- a/tests/src/Formatter/MarkdownFormatterTest.php +++ b/tests/src/Formatter/MarkdownFormatterTest.php @@ -224,14 +224,16 @@ public function testMergeRequestListResult(): void ); $result = new MergeRequestListResult( - projectPath: 'issue/drupal-3383637', + projectPath: 'project/drupal', mergeRequests: [$mr], + issueFork: 'issue/drupal-3383637', ); $formatter = new MarkdownFormatter(); $output = $formatter->format($result); - self::assertStringContainsString('# Merge Requests: issue/drupal-3383637', $output); + self::assertStringContainsString('# Merge Requests: project/drupal', $output); + self::assertStringContainsString('Scoped to issue fork `issue/drupal-3383637`.', $output); self::assertStringContainsString('**!7**', $output); self::assertStringContainsString('[Fix the bug](https://git.drupalcode.org/issue/drupal-3383637/-/merge_requests/7)', $output); self::assertStringContainsString('[opened ✓]', $output);