From 91c78fd337ceed2d4f57e15410dba852cd57618d Mon Sep 17 00:00:00 2001 From: Cascade Bot Date: Mon, 23 Feb 2026 11:41:04 +0000 Subject: [PATCH] feat(pm): replace PR comment links with native attachments/remote links --- src/jira/client.ts | 17 +++++ src/pm/jira/adapter.ts | 4 ++ src/pm/lifecycle.ts | 30 +++++++-- src/pm/trello/adapter.ts | 4 ++ src/pm/types.ts | 3 + tests/helpers/mockPMProvider.ts | 1 + tests/unit/jira/client.test.ts | 71 +++++++++++++++++++++ tests/unit/pm/jira/adapter.test.ts | 15 +++++ tests/unit/pm/lifecycle.test.ts | 92 +++++++++++++++++++++------- tests/unit/pm/trello/adapter.test.ts | 14 +++++ 10 files changed, 225 insertions(+), 26 deletions(-) diff --git a/src/jira/client.ts b/src/jira/client.ts index 7a20fce7..0fc0e1fb 100644 --- a/src/jira/client.ts +++ b/src/jira/client.ts @@ -253,4 +253,21 @@ export const jiraClient = { }, }); }, + + async addRemoteLink(issueKey: string, url: string, title: string): Promise { + logger.debug('Adding JIRA remote link', { issueKey, url, title }); + await getClient().issueRemoteLinks.createOrUpdateRemoteIssueLink({ + issueIdOrKey: issueKey, + globalId: url, + relationship: 'Pull Request', + object: { + url, + title, + icon: { + url16x16: 'https://github.com/favicon.ico', + title: 'GitHub', + }, + }, + }); + }, }; diff --git a/src/pm/jira/adapter.ts b/src/pm/jira/adapter.ts index 3bb983a4..34786c4d 100644 --- a/src/pm/jira/adapter.ts +++ b/src/pm/jira/adapter.ts @@ -331,6 +331,10 @@ export class JiraPMProvider implements PMProvider { await this.addComment(_workItemId, `Attachment: [${name}](${url})`); } + async linkPR(workItemId: string, prUrl: string, prTitle: string): Promise { + await jiraClient.addRemoteLink(workItemId, prUrl, prTitle); + } + async addAttachmentFile( workItemId: string, buffer: Buffer, diff --git a/src/pm/lifecycle.ts b/src/pm/lifecycle.ts index cbd3c6c5..8880c0c6 100644 --- a/src/pm/lifecycle.ts +++ b/src/pm/lifecycle.ts @@ -29,6 +29,15 @@ export interface ProjectPMConfig { }; } +/** + * Extract a human-readable PR title from a GitHub PR URL. + * E.g. "https://github.com/owner/repo/pull/123" → "Pull Request #123" + */ +export function extractPRTitle(prUrl: string): string { + const match = prUrl.match(/\/pull\/(\d+)/); + return match ? `Pull Request #${match[1]}` : 'Pull Request'; +} + /** * Resolve PM-specific config (labels, statuses) from project configuration. * Delegates to the registered integration's resolveLifecycleConfig(). @@ -64,11 +73,22 @@ export class PMLifecycleManager { if (agentType === 'implementation') { await this.safeMove(workItemId, this.pmConfig.statuses.inReview); if (prUrl) { - if (progressCommentId) { - // Replace the progress comment with the "PR created" message - await this.safeUpdateOrAddComment(workItemId, progressCommentId, `PR created: ${prUrl}`); - } else { - await this.safeAddComment(workItemId, `PR created: ${prUrl}`); + const prTitle = extractPRTitle(prUrl); + let linked = false; + try { + await this.provider.linkPR(workItemId, prUrl, prTitle); + linked = true; + } catch { + // linkPR failed — fall through to comment fallback + } + if (!linked) { + const message = `PR created: ${prUrl}`; + if (progressCommentId) { + // Replace the progress comment with the "PR created" message + await this.safeUpdateOrAddComment(workItemId, progressCommentId, message); + } else { + await this.safeAddComment(workItemId, message); + } } } } diff --git a/src/pm/trello/adapter.ts b/src/pm/trello/adapter.ts index ff42461f..f879c576 100644 --- a/src/pm/trello/adapter.ts +++ b/src/pm/trello/adapter.ts @@ -196,6 +196,10 @@ export class TrelloPMProvider implements PMProvider { await trelloClient.addAttachment(workItemId, url, name); } + async linkPR(workItemId: string, prUrl: string, prTitle: string): Promise { + await trelloClient.addAttachment(workItemId, prUrl, prTitle); + } + async addAttachmentFile( workItemId: string, buffer: Buffer, diff --git a/src/pm/types.ts b/src/pm/types.ts index 5c90a3d6..00cd326d 100644 --- a/src/pm/types.ts +++ b/src/pm/types.ts @@ -101,6 +101,9 @@ export interface PMProvider { getCustomFieldNumber(workItemId: string, fieldId: string): Promise; updateCustomFieldNumber(workItemId: string, fieldId: string, value: number): Promise; + // PR linking + linkPR(workItemId: string, prUrl: string, prTitle: string): Promise; + // Utility getWorkItemUrl(id: string): string; getAuthenticatedUser(): Promise<{ id: string; name: string; username: string }>; diff --git a/tests/helpers/mockPMProvider.ts b/tests/helpers/mockPMProvider.ts index 573ba462..c846cdc1 100644 --- a/tests/helpers/mockPMProvider.ts +++ b/tests/helpers/mockPMProvider.ts @@ -33,6 +33,7 @@ export function createMockPMProvider() { deleteChecklistItem: vi.fn(), addAttachment: vi.fn(), addAttachmentFile: vi.fn(), + linkPR: vi.fn().mockResolvedValue(undefined), getCustomFieldNumber: vi.fn(), updateCustomFieldNumber: vi.fn(), getWorkItemUrl: vi.fn(), diff --git a/tests/unit/jira/client.test.ts b/tests/unit/jira/client.test.ts index 23413dcb..db763e9d 100644 --- a/tests/unit/jira/client.test.ts +++ b/tests/unit/jira/client.test.ts @@ -15,6 +15,7 @@ const { mockIssueComments, mockIssueSearch, mockIssueAttachments, + mockIssueRemoteLinks, mockMyself, mockProjects, } = vi.hoisted(() => ({ @@ -37,6 +38,9 @@ const { mockIssueAttachments: { addAttachment: vi.fn(), }, + mockIssueRemoteLinks: { + createOrUpdateRemoteIssueLink: vi.fn(), + }, mockMyself: { getCurrentUser: vi.fn(), }, @@ -51,6 +55,7 @@ vi.mock('jira.js', () => ({ issueComments: mockIssueComments, issueSearch: mockIssueSearch, issueAttachments: mockIssueAttachments, + issueRemoteLinks: mockIssueRemoteLinks, myself: mockMyself, projects: mockProjects, })), @@ -84,6 +89,7 @@ describe('jiraClient', () => { mockIssueComments.updateComment.mockReset(); mockIssueSearch.searchForIssuesUsingJql.mockReset(); mockIssueAttachments.addAttachment.mockReset(); + mockIssueRemoteLinks.createOrUpdateRemoteIssueLink.mockReset(); mockMyself.getCurrentUser.mockReset(); mockProjects.getProject.mockReset(); _resetCloudIdCache(); @@ -567,6 +573,71 @@ describe('jiraClient', () => { }); }); + describe('addRemoteLink', () => { + it('calls createOrUpdateRemoteIssueLink with correct params', async () => { + mockIssueRemoteLinks.createOrUpdateRemoteIssueLink.mockResolvedValue({ id: 'link-1' }); + + await withJiraCredentials(creds, () => + jiraClient.addRemoteLink( + 'TEST-1', + 'https://github.com/owner/repo/pull/42', + 'Pull Request #42', + ), + ); + + expect(mockIssueRemoteLinks.createOrUpdateRemoteIssueLink).toHaveBeenCalledWith( + expect.objectContaining({ + issueIdOrKey: 'TEST-1', + globalId: 'https://github.com/owner/repo/pull/42', + relationship: 'Pull Request', + object: expect.objectContaining({ + url: 'https://github.com/owner/repo/pull/42', + title: 'Pull Request #42', + }), + }), + ); + }); + + it('uses PR URL as globalId for idempotency', async () => { + mockIssueRemoteLinks.createOrUpdateRemoteIssueLink.mockResolvedValue({ id: 'link-2' }); + const prUrl = 'https://github.com/owner/repo/pull/99'; + + await withJiraCredentials(creds, () => + jiraClient.addRemoteLink('PROJ-5', prUrl, 'Pull Request #99'), + ); + + expect(mockIssueRemoteLinks.createOrUpdateRemoteIssueLink).toHaveBeenCalledWith( + expect.objectContaining({ + globalId: prUrl, + }), + ); + }); + + it('sets GitHub favicon icon on the remote link object', async () => { + mockIssueRemoteLinks.createOrUpdateRemoteIssueLink.mockResolvedValue({}); + + await withJiraCredentials(creds, () => + jiraClient.addRemoteLink('TEST-1', 'https://github.com/owner/repo/pull/1', 'PR #1'), + ); + + expect(mockIssueRemoteLinks.createOrUpdateRemoteIssueLink).toHaveBeenCalledWith( + expect.objectContaining({ + object: expect.objectContaining({ + icon: expect.objectContaining({ + url16x16: 'https://github.com/favicon.ico', + }), + }), + }), + ); + }); + + it('throws when called outside withJiraCredentials scope', async () => { + await expect( + jiraClient.addRemoteLink('TEST-1', 'https://github.com/pr/1', 'PR #1'), + ).rejects.toThrow('No JIRA credentials in scope'); + }); + }); + describe('getIssueComments', () => { it('returns comments array', async () => { const comments = [{ id: 'c1', body: 'First comment' }]; diff --git a/tests/unit/pm/jira/adapter.test.ts b/tests/unit/pm/jira/adapter.test.ts index d6a93135..e5609f12 100644 --- a/tests/unit/pm/jira/adapter.test.ts +++ b/tests/unit/pm/jira/adapter.test.ts @@ -17,6 +17,7 @@ const { mockJiraClient, mockAdfToPlainText, mockMarkdownToAdf } = vi.hoisted(() getIssueLabels: vi.fn(), updateLabels: vi.fn(), addAttachmentFile: vi.fn(), + addRemoteLink: vi.fn(), getCustomFieldValue: vi.fn(), updateCustomField: vi.fn(), getMyself: vi.fn(), @@ -685,6 +686,20 @@ describe('JiraPMProvider', () => { }); }); + describe('linkPR', () => { + it('delegates to jiraClient.addRemoteLink with workItemId, prUrl, and prTitle', async () => { + mockJiraClient.addRemoteLink.mockResolvedValue(undefined); + + await provider.linkPR('PROJ-1', 'https://github.com/owner/repo/pull/42', 'Pull Request #42'); + + expect(mockJiraClient.addRemoteLink).toHaveBeenCalledWith( + 'PROJ-1', + 'https://github.com/owner/repo/pull/42', + 'Pull Request #42', + ); + }); + }); + describe('getCustomFieldNumber', () => { it('returns numeric custom field value', async () => { mockJiraClient.getCustomFieldValue.mockResolvedValue(99); diff --git a/tests/unit/pm/lifecycle.test.ts b/tests/unit/pm/lifecycle.test.ts index f45c7374..34e190d9 100644 --- a/tests/unit/pm/lifecycle.test.ts +++ b/tests/unit/pm/lifecycle.test.ts @@ -28,12 +28,33 @@ import '../../../src/pm/index.js'; import { PMLifecycleManager, type ProjectPMConfig, + extractPRTitle, resolveProjectPMConfig, } from '../../../src/pm/lifecycle.js'; import type { PMProvider } from '../../../src/pm/types.js'; import type { ProjectConfig } from '../../../src/types/index.js'; describe('pm/lifecycle', () => { + describe('extractPRTitle', () => { + it('extracts PR number from a standard GitHub PR URL', () => { + expect(extractPRTitle('https://github.com/owner/repo/pull/123')).toBe('Pull Request #123'); + }); + + it('extracts PR number from a PR URL with trailing path', () => { + expect(extractPRTitle('https://github.com/owner/repo/pull/42/files')).toBe( + 'Pull Request #42', + ); + }); + + it('returns generic title when URL does not contain /pull/', () => { + expect(extractPRTitle('https://example.com/no-pull-here')).toBe('Pull Request'); + }); + + it('returns generic title for empty string', () => { + expect(extractPRTitle('')).toBe('Pull Request'); + }); + }); + describe('resolveProjectPMConfig', () => { it('returns JIRA config when project type is jira', () => { const project: ProjectConfig = { @@ -275,6 +296,7 @@ describe('pm/lifecycle', () => { moveWorkItem: vi.fn().mockResolvedValue(undefined), addComment: vi.fn().mockResolvedValue(undefined), updateComment: vi.fn().mockResolvedValue(undefined), + linkPR: vi.fn().mockResolvedValue(undefined), // Other PMProvider methods (not used by lifecycle manager) getWorkItem: vi.fn(), getWorkItemComments: vi.fn(), @@ -361,18 +383,31 @@ describe('pm/lifecycle', () => { expect(mockProvider.moveWorkItem).toHaveBeenCalledWith('work-item-1', 'list-review'); }); - it('adds PR comment when prUrl is provided for implementation agent', async () => { - await manager.handleSuccess('work-item-1', 'implementation', 'https://github.com/pr/123'); + it('calls linkPR when prUrl is provided for implementation agent', async () => { + await manager.handleSuccess( + 'work-item-1', + 'implementation', + 'https://github.com/owner/repo/pull/123', + ); - expect(mockProvider.addComment).toHaveBeenCalledWith( + expect(mockProvider.linkPR).toHaveBeenCalledWith( 'work-item-1', - 'PR created: https://github.com/pr/123', + 'https://github.com/owner/repo/pull/123', + 'Pull Request #123', ); }); - it('does not add PR comment when prUrl is not provided', async () => { + it('does not post comment when linkPR succeeds', async () => { + await manager.handleSuccess('work-item-1', 'implementation', 'https://github.com/pr/123'); + + expect(mockProvider.addComment).not.toHaveBeenCalled(); + expect(mockProvider.updateComment).not.toHaveBeenCalled(); + }); + + it('does not call linkPR when prUrl is not provided', async () => { await manager.handleSuccess('work-item-1', 'implementation'); + expect(mockProvider.linkPR).not.toHaveBeenCalled(); expect(mockProvider.addComment).not.toHaveBeenCalled(); }); @@ -382,24 +417,34 @@ describe('pm/lifecycle', () => { expect(mockProvider.moveWorkItem).not.toHaveBeenCalled(); }); - it('updates existing progress comment when progressCommentId provided', async () => { + it('does not call linkPR for non-implementation agents even with prUrl', async () => { + await manager.handleSuccess('work-item-1', 'briefing', 'https://github.com/pr/123'); + + expect(mockProvider.linkPR).not.toHaveBeenCalled(); + }); + + it('falls back to addComment when linkPR fails and no progressCommentId', async () => { + vi.mocked(mockProvider.linkPR).mockRejectedValue(new Error('Permission denied')); + await manager.handleSuccess( 'work-item-1', 'implementation', - 'https://github.com/pr/123', - 'comment-abc', + 'https://github.com/owner/repo/pull/123', ); - expect(mockProvider.updateComment).toHaveBeenCalledWith( + expect(mockProvider.linkPR).toHaveBeenCalledWith( 'work-item-1', - 'comment-abc', - 'PR created: https://github.com/pr/123', + 'https://github.com/owner/repo/pull/123', + 'Pull Request #123', + ); + expect(mockProvider.addComment).toHaveBeenCalledWith( + 'work-item-1', + 'PR created: https://github.com/owner/repo/pull/123', ); - expect(mockProvider.addComment).not.toHaveBeenCalled(); }); - it('falls back to addComment when updateComment fails', async () => { - vi.mocked(mockProvider.updateComment).mockRejectedValue(new Error('Comment not found')); + it('falls back to updateComment when linkPR fails and progressCommentId provided', async () => { + vi.mocked(mockProvider.linkPR).mockRejectedValue(new Error('Permission denied')); await manager.handleSuccess( 'work-item-1', @@ -408,25 +453,30 @@ describe('pm/lifecycle', () => { 'comment-abc', ); + expect(mockProvider.linkPR).toHaveBeenCalled(); expect(mockProvider.updateComment).toHaveBeenCalledWith( 'work-item-1', 'comment-abc', 'PR created: https://github.com/pr/123', ); - expect(mockProvider.addComment).toHaveBeenCalledWith( - 'work-item-1', - 'PR created: https://github.com/pr/123', - ); + expect(mockProvider.addComment).not.toHaveBeenCalled(); }); - it('uses addComment when progressCommentId is not provided', async () => { - await manager.handleSuccess('work-item-1', 'implementation', 'https://github.com/pr/123'); + it('falls back to addComment when linkPR fails and updateComment also fails', async () => { + vi.mocked(mockProvider.linkPR).mockRejectedValue(new Error('Permission denied')); + vi.mocked(mockProvider.updateComment).mockRejectedValue(new Error('Comment not found')); + + await manager.handleSuccess( + 'work-item-1', + 'implementation', + 'https://github.com/pr/123', + 'comment-abc', + ); expect(mockProvider.addComment).toHaveBeenCalledWith( 'work-item-1', 'PR created: https://github.com/pr/123', ); - expect(mockProvider.updateComment).not.toHaveBeenCalled(); }); }); diff --git a/tests/unit/pm/trello/adapter.test.ts b/tests/unit/pm/trello/adapter.test.ts index 5072d3c7..d26585f6 100644 --- a/tests/unit/pm/trello/adapter.test.ts +++ b/tests/unit/pm/trello/adapter.test.ts @@ -419,6 +419,20 @@ describe('TrelloPMProvider', () => { }); }); + describe('linkPR', () => { + it('delegates to trelloClient.addAttachment with prUrl and prTitle', async () => { + mockTrelloClient.addAttachment.mockResolvedValue(undefined); + + await provider.linkPR('card-1', 'https://github.com/owner/repo/pull/42', 'Pull Request #42'); + + expect(mockTrelloClient.addAttachment).toHaveBeenCalledWith( + 'card-1', + 'https://github.com/owner/repo/pull/42', + 'Pull Request #42', + ); + }); + }); + describe('getCustomFieldNumber', () => { it('returns the parsed number from custom field items', async () => { mockTrelloClient.getCardCustomFieldItems.mockResolvedValue([