Skip to content

Commit 20f6f3c

Browse files
costajohntcostajohnt
authored
fix: dismiss command accepts PR URLs and filters them from actionable items (#416) (#464)
The dismiss/undismiss commands only accepted issue URLs (/issues/123), rejecting PR URLs (/pull/123). PRs that didn't need action kept resurfacing with no way to suppress them. - Add ISSUE_OR_PR_URL_PATTERN that accepts both /issues/ and /pull/ URLs - Wire dismissed PR URLs into the daily digest's actionable issue filter so dismissed PRs are actually excluded from notifications - Rename issueUrl parameter to url throughout the dismiss command chain - Update CLI help text, MCP server tool descriptions, and error messages - Consolidate URL pattern tests in validation.test.ts
1 parent 4b8f664 commit 20f6f3c

File tree

8 files changed

+142
-72
lines changed

8 files changed

+142
-72
lines changed

‎packages/core/src/cli.ts‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -696,21 +696,21 @@ program
696696

697697
// Dismiss command
698698
program
699-
.command('dismiss <issue-url>')
700-
.description('Dismiss issue reply notifications (resurfaces on new activity)')
699+
.command('dismiss <url>')
700+
.description('Dismiss notifications for an issue or PR (resurfaces on new activity)')
701701
.option('--json', 'Output as JSON')
702-
.action(async (issueUrl, options) => {
702+
.action(async (url, options) => {
703703
try {
704704
const { runDismiss } = await import('./commands/dismiss.js');
705-
const data = await runDismiss({ issueUrl });
705+
const data = await runDismiss({ url });
706706
if (options.json) {
707707
outputJson(data);
708708
} else if (data.dismissed) {
709-
console.log(`Dismissed: ${issueUrl}`);
710-
console.log('Issue reply notifications are now muted.');
709+
console.log(`Dismissed: ${url}`);
710+
console.log('Notifications are now muted.');
711711
console.log('New responses after this point will resurface automatically.');
712712
} else {
713-
console.log('Issue is already dismissed.');
713+
console.log('Already dismissed.');
714714
}
715715
} catch (err) {
716716
handleCommandError(err, options.json);
@@ -719,20 +719,20 @@ program
719719

720720
// Undismiss command
721721
program
722-
.command('undismiss <issue-url>')
723-
.description('Undismiss an issue (re-enable reply notifications)')
722+
.command('undismiss <url>')
723+
.description('Undismiss an issue or PR (re-enable notifications)')
724724
.option('--json', 'Output as JSON')
725-
.action(async (issueUrl, options) => {
725+
.action(async (url, options) => {
726726
try {
727727
const { runUndismiss } = await import('./commands/dismiss.js');
728-
const data = await runUndismiss({ issueUrl });
728+
const data = await runUndismiss({ url });
729729
if (options.json) {
730730
outputJson(data);
731731
} else if (data.undismissed) {
732-
console.log(`Undismissed: ${issueUrl}`);
733-
console.log('Issue reply notifications are active again.');
732+
console.log(`Undismissed: ${url}`);
733+
console.log('Notifications are active again.');
734734
} else {
735-
console.log('Issue was not dismissed.');
735+
console.log('Was not dismissed.');
736736
}
737737
} catch (err) {
738738
handleCommandError(err, options.json);

‎packages/core/src/commands/daily-orchestration.test.ts‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -560,6 +560,43 @@ describe('executeDailyCheck() — snoozed PR filtering', () => {
560560
});
561561
});
562562

563+
// ---------------------------------------------------------------------------
564+
// executeDailyCheck() — dismissed PR URL filtering (#416)
565+
// ---------------------------------------------------------------------------
566+
567+
describe('executeDailyCheck() — dismissed PR URL filtering (#416)', () => {
568+
it('excludes dismissed PR URLs from actionableIssues', async () => {
569+
const dismissedPR = makePR({ repo: 'owner/repo', number: 1, status: 'needs_response' });
570+
const activePR = makePR({ repo: 'owner/repo', number: 2, status: 'needs_response' });
571+
mockFetchUserOpenPRs.mockResolvedValue({ prs: [dismissedPR, activePR], failures: [] });
572+
mockGenerateDigest.mockReturnValue(makeDigest([dismissedPR, activePR]));
573+
mockIsPRShelved.mockReturnValue(false);
574+
575+
mockGetState.mockReturnValue(
576+
makeDefaultState({
577+
config: {
578+
setupComplete: true,
579+
githubUsername: 'testuser',
580+
maxActivePRs: 10,
581+
languages: [],
582+
labels: [],
583+
excludeRepos: [],
584+
trustedProjects: [],
585+
shelvedPRUrls: [],
586+
dismissedIssues: { [dismissedPR.url]: '2026-01-01T00:00:00Z' },
587+
snoozedPRs: {},
588+
},
589+
}),
590+
);
591+
592+
const result = await executeDailyCheck('test-token');
593+
594+
const needsResponseIssues = result.actionableIssues.filter((i) => i.type === 'needs_response');
595+
expect(needsResponseIssues).toHaveLength(1);
596+
expect(needsResponseIssues[0].prUrl).toBe(activePR.url);
597+
});
598+
});
599+
563600
// ---------------------------------------------------------------------------
564601
// executeDailyCheck() — JSON output shape
565602
// ---------------------------------------------------------------------------

‎packages/core/src/commands/daily.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -473,7 +473,10 @@ function generateDigestOutput(
473473
const snoozedUrls = new Set(
474474
Object.keys(stateManager.getState().config.snoozedPRs ?? {}).filter((url) => stateManager.isSnoozed(url)),
475475
);
476-
const actionableIssues = collectActionableIssues(activePRs, snoozedUrls);
476+
// Filter dismissed PR URLs from actionable issues (#416)
477+
const dismissedUrls = new Set(Object.keys(stateManager.getState().config.dismissedIssues ?? {}));
478+
const nonDismissedPRs = activePRs.filter((pr) => !dismissedUrls.has(pr.url));
479+
const actionableIssues = collectActionableIssues(nonDismissedPRs, snoozedUrls);
477480
digest.summary.totalNeedingAttention = actionableIssues.length;
478481
const briefSummary = formatBriefSummary(digest, actionableIssues.length, issueResponses.length);
479482
const actionMenu = computeActionMenu(actionableIssues, capacity, filteredCommentedIssues);

‎packages/core/src/commands/dismiss.test.ts‎

Lines changed: 24 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -3,34 +3,7 @@
33
*/
44

55
import { describe, it, expect, vi, beforeEach } from 'vitest';
6-
import { ISSUE_URL_PATTERN } from './dismiss.js';
7-
8-
describe('ISSUE_URL_PATTERN', () => {
9-
it.each([
10-
['https://github.com/owner/repo/issues/123', 'standard URL'],
11-
['https://github.com/my-org/my-repo/issues/1', 'hyphenated names'],
12-
['https://github.com/owner/repo.js/issues/42', 'dotted repo name'],
13-
['https://github.com/my_org/my_repo/issues/7', 'underscored names'],
14-
['https://github.com/owner/repo/issues/99999', 'large issue number'],
15-
])('should match %s (%s)', (url) => {
16-
expect(ISSUE_URL_PATTERN.test(url)).toBe(true);
17-
});
18-
19-
it.each([
20-
['https://github.com/owner/repo/pull/123', 'PR URL'],
21-
['https://gitlab.com/owner/repo/issues/1', 'non-GitHub host'],
22-
['http://github.com/owner/repo/issues/1', 'HTTP (non-HTTPS)'],
23-
['https://github.com/owner/repo/issues/123/', 'trailing slash'],
24-
['https://github.com/owner/repo/issues/123?q=test', 'query parameters'],
25-
['https://github.com/owner/repo/issues/123#comment', 'fragment identifier'],
26-
['https://github.com/owner/repo/issues/', 'missing issue number'],
27-
['https://github.com/owner/repo', 'bare repo URL'],
28-
['', 'empty string'],
29-
['https://github.com/owner/repo/issues/123/timeline', 'extra path segments'],
30-
])('should reject %s (%s)', (url) => {
31-
expect(ISSUE_URL_PATTERN.test(url)).toBe(false);
32-
});
33-
});
6+
// Pattern tests are in validation.test.ts (canonical home for URL patterns)
347

358
// Mock getStateManager for command-level tests
369
vi.mock('../core/index.js', () => ({
@@ -43,6 +16,7 @@ import { runDismiss, runUndismiss } from './dismiss.js';
4316
const mockGetStateManager = vi.mocked(getStateManager);
4417

4518
const TEST_ISSUE_URL = 'https://github.com/owner/repo/issues/1';
19+
const TEST_PR_URL = 'https://github.com/owner/repo/pull/42';
4620

4721
describe('runDismiss', () => {
4822
const mockSave = vi.fn();
@@ -58,16 +32,25 @@ describe('runDismiss', () => {
5832

5933
it('should dismiss an issue and save state', async () => {
6034
mockDismissIssue.mockReturnValue(true);
61-
const result = await runDismiss({ issueUrl: TEST_ISSUE_URL });
35+
const result = await runDismiss({ url: TEST_ISSUE_URL });
6236

6337
expect(mockDismissIssue).toHaveBeenCalledWith(TEST_ISSUE_URL, expect.any(String));
6438
expect(mockSave).toHaveBeenCalled();
6539
expect(result).toEqual({ dismissed: true, url: TEST_ISSUE_URL });
6640
});
6741

42+
it('should dismiss a PR URL and save state (#416)', async () => {
43+
mockDismissIssue.mockReturnValue(true);
44+
const result = await runDismiss({ url: TEST_PR_URL });
45+
46+
expect(mockDismissIssue).toHaveBeenCalledWith(TEST_PR_URL, expect.any(String));
47+
expect(mockSave).toHaveBeenCalled();
48+
expect(result).toEqual({ dismissed: true, url: TEST_PR_URL });
49+
});
50+
6851
it('should not save state when issue is already dismissed', async () => {
6952
mockDismissIssue.mockReturnValue(false);
70-
const result = await runDismiss({ issueUrl: TEST_ISSUE_URL });
53+
const result = await runDismiss({ url: TEST_ISSUE_URL });
7154

7255
expect(mockSave).not.toHaveBeenCalled();
7356
expect(result).toEqual({ dismissed: false, url: TEST_ISSUE_URL });
@@ -88,16 +71,25 @@ describe('runUndismiss', () => {
8871

8972
it('should undismiss an issue and save state', async () => {
9073
mockUndismissIssue.mockReturnValue(true);
91-
const result = await runUndismiss({ issueUrl: TEST_ISSUE_URL });
74+
const result = await runUndismiss({ url: TEST_ISSUE_URL });
9275

9376
expect(mockUndismissIssue).toHaveBeenCalledWith(TEST_ISSUE_URL);
9477
expect(mockSave).toHaveBeenCalled();
9578
expect(result).toEqual({ undismissed: true, url: TEST_ISSUE_URL });
9679
});
9780

81+
it('should undismiss a PR URL and save state (#416)', async () => {
82+
mockUndismissIssue.mockReturnValue(true);
83+
const result = await runUndismiss({ url: TEST_PR_URL });
84+
85+
expect(mockUndismissIssue).toHaveBeenCalledWith(TEST_PR_URL);
86+
expect(mockSave).toHaveBeenCalled();
87+
expect(result).toEqual({ undismissed: true, url: TEST_PR_URL });
88+
});
89+
9890
it('should not save state when issue was not dismissed', async () => {
9991
mockUndismissIssue.mockReturnValue(false);
100-
const result = await runUndismiss({ issueUrl: TEST_ISSUE_URL });
92+
const result = await runUndismiss({ url: TEST_ISSUE_URL });
10193

10294
expect(mockSave).not.toHaveBeenCalled();
10395
expect(result).toEqual({ undismissed: false, url: TEST_ISSUE_URL });

‎packages/core/src/commands/dismiss.ts‎

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
/**
22
* Dismiss/Undismiss commands
3-
* Manages dismissing issue reply notifications without posting a comment.
4-
* Dismissed issues resurface automatically when new responses arrive after the dismiss timestamp.
3+
* Manages dismissing issue and PR notifications without posting a comment.
4+
* Dismissed URLs resurface automatically when new responses arrive after the dismiss timestamp.
55
*/
66

77
import { getStateManager } from '../core/index.js';
8-
import { ISSUE_URL_PATTERN, validateGitHubUrl, validateUrl } from './validation.js';
8+
import { ISSUE_OR_PR_URL_PATTERN, validateGitHubUrl, validateUrl } from './validation.js';
99

1010
export interface DismissOutput {
1111
dismissed: boolean;
@@ -17,33 +17,33 @@ export interface UndismissOutput {
1717
url: string;
1818
}
1919

20-
// Re-export for backward compatibility with tests
21-
export { ISSUE_URL_PATTERN };
20+
// Re-export for tests
21+
export { ISSUE_OR_PR_URL_PATTERN };
2222

23-
export async function runDismiss(options: { issueUrl: string }): Promise<DismissOutput> {
24-
validateUrl(options.issueUrl);
25-
validateGitHubUrl(options.issueUrl, ISSUE_URL_PATTERN, 'issue');
23+
export async function runDismiss(options: { url: string }): Promise<DismissOutput> {
24+
validateUrl(options.url);
25+
validateGitHubUrl(options.url, ISSUE_OR_PR_URL_PATTERN, 'issue or PR');
2626

2727
const stateManager = getStateManager();
28-
const added = stateManager.dismissIssue(options.issueUrl, new Date().toISOString());
28+
const added = stateManager.dismissIssue(options.url, new Date().toISOString());
2929

3030
if (added) {
3131
stateManager.save();
3232
}
3333

34-
return { dismissed: added, url: options.issueUrl };
34+
return { dismissed: added, url: options.url };
3535
}
3636

37-
export async function runUndismiss(options: { issueUrl: string }): Promise<UndismissOutput> {
38-
validateUrl(options.issueUrl);
39-
validateGitHubUrl(options.issueUrl, ISSUE_URL_PATTERN, 'issue');
37+
export async function runUndismiss(options: { url: string }): Promise<UndismissOutput> {
38+
validateUrl(options.url);
39+
validateGitHubUrl(options.url, ISSUE_OR_PR_URL_PATTERN, 'issue or PR');
4040

4141
const stateManager = getStateManager();
42-
const removed = stateManager.undismissIssue(options.issueUrl);
42+
const removed = stateManager.undismissIssue(options.url);
4343

4444
if (removed) {
4545
stateManager.save();
4646
}
4747

48-
return { undismissed: removed, url: options.issueUrl };
48+
return { undismissed: removed, url: options.url };
4949
}

‎packages/core/src/commands/validation.test.ts‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import { describe, it, expect } from 'vitest';
77
import {
88
PR_URL_PATTERN,
99
ISSUE_URL_PATTERN,
10+
ISSUE_OR_PR_URL_PATTERN,
1011
validateUrl,
1112
validatePRNumber,
1213
validateMessage,
@@ -46,6 +47,28 @@ describe('ISSUE_URL_PATTERN', () => {
4647
});
4748
});
4849

50+
describe('ISSUE_OR_PR_URL_PATTERN (#416)', () => {
51+
it.each([
52+
['https://github.com/owner/repo/issues/123', 'issue URL'],
53+
['https://github.com/owner/repo/pull/123', 'PR URL'],
54+
['https://github.com/my-org/my-repo/pull/1', 'hyphenated PR'],
55+
['https://github.com/owner/repo.js/pull/42', 'dotted repo PR'],
56+
])('should match %s (%s)', (url) => {
57+
expect(ISSUE_OR_PR_URL_PATTERN.test(url)).toBe(true);
58+
});
59+
60+
it.each([
61+
['https://gitlab.com/owner/repo/pull/1', 'non-GitHub host'],
62+
['http://github.com/owner/repo/pull/1', 'HTTP (non-HTTPS)'],
63+
['https://github.com/owner/repo/pull/123/', 'trailing slash'],
64+
['https://github.com/owner/repo/pulls', 'pulls listing'],
65+
['https://github.com/owner/repo', 'bare repo URL'],
66+
['', 'empty string'],
67+
])('should reject %s (%s)', (url) => {
68+
expect(ISSUE_OR_PR_URL_PATTERN.test(url)).toBe(false);
69+
});
70+
});
71+
4972
describe('validateUrl', () => {
5073
it('should return valid URLs unchanged', () => {
5174
const url = 'https://github.com/owner/repo/pull/123';
@@ -149,6 +172,15 @@ describe('validateGitHubUrl', () => {
149172
'Expected format: https://github.com/owner/repo/pull/123',
150173
);
151174
});
175+
176+
it('should throw with combined format for issue or PR (#416)', () => {
177+
expect(() => validateGitHubUrl('bad-url', ISSUE_OR_PR_URL_PATTERN, 'issue or PR')).toThrow(
178+
'Invalid issue or PR URL',
179+
);
180+
expect(() => validateGitHubUrl('bad-url', ISSUE_OR_PR_URL_PATTERN, 'issue or PR')).toThrow(
181+
'Expected format: https://github.com/owner/repo/issues/123 or https://github.com/owner/repo/pull/123',
182+
);
183+
});
152184
});
153185

154186
describe('validateGitHubUsername', () => {

‎packages/core/src/commands/validation.ts‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,9 @@ export const PR_URL_PATTERN = /^https:\/\/github\.com\/[^/]+\/[^/]+\/pull\/\d+$/
1010
/** Matches GitHub issue URLs: https://github.com/owner/repo/issues/123 */
1111
export const ISSUE_URL_PATTERN = /^https:\/\/github\.com\/[^/]+\/[^/]+\/issues\/\d+$/;
1212

13+
/** Matches GitHub issue or PR URLs: /issues/123 or /pull/123 */
14+
export const ISSUE_OR_PR_URL_PATTERN = /^https:\/\/github\.com\/[^/]+\/[^/]+\/(issues|pull)\/\d+$/;
15+
1316
/** Maximum allowed URL length */
1417
const MAX_URL_LENGTH = 2048;
1518
/** Maximum allowed PR/issue number */
@@ -22,12 +25,15 @@ const REPO_PATTERN = /^[a-zA-Z0-9._-]+\/[a-zA-Z0-9._-]+$/;
2225
/**
2326
* Validate a GitHub URL against a pattern. Throws if invalid.
2427
*/
25-
export function validateGitHubUrl(url: string, pattern: RegExp, entityType: 'PR' | 'issue'): void {
28+
export function validateGitHubUrl(url: string, pattern: RegExp, entityType: 'PR' | 'issue' | 'issue or PR'): void {
2629
if (pattern.test(url)) return;
2730

28-
const example =
29-
entityType === 'PR' ? 'https://github.com/owner/repo/pull/123' : 'https://github.com/owner/repo/issues/123';
30-
throw new ValidationError(`Invalid ${entityType} URL: ${url}. Expected format: ${example}`);
31+
const examples: Record<string, string> = {
32+
PR: 'https://github.com/owner/repo/pull/123',
33+
issue: 'https://github.com/owner/repo/issues/123',
34+
'issue or PR': 'https://github.com/owner/repo/issues/123 or https://github.com/owner/repo/pull/123',
35+
};
36+
throw new ValidationError(`Invalid ${entityType} URL: ${url}. Expected format: ${examples[entityType]}`);
3137
}
3238

3339
/**

‎packages/mcp-server/src/tools.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -301,26 +301,26 @@ export function registerTools(server: McpServer): void {
301301
wrapTool(runUnshelve),
302302
);
303303

304-
// 18. dismiss — Dismiss an issue
304+
// 18. dismiss — Dismiss an issue or PR
305305
server.registerTool(
306306
'dismiss',
307307
{
308-
description: 'Dismiss a GitHub issue so it no longer appears in search results.',
308+
description: 'Dismiss a GitHub issue or PR so it no longer appears in notifications.',
309309
inputSchema: {
310-
issueUrl: z.string().describe('Full GitHub issue URL to dismiss'),
310+
url: z.string().describe('Full GitHub issue or PR URL to dismiss'),
311311
},
312312
annotations: { readOnlyHint: false, destructiveHint: false },
313313
},
314314
wrapTool(runDismiss),
315315
);
316316

317-
// 19. undismiss — Undismiss an issue
317+
// 19. undismiss — Undismiss an issue or PR
318318
server.registerTool(
319319
'undismiss',
320320
{
321-
description: 'Undismiss a previously dismissed issue, allowing it to appear in search results again.',
321+
description: 'Undismiss a previously dismissed issue or PR, re-enabling notifications.',
322322
inputSchema: {
323-
issueUrl: z.string().describe('Full GitHub issue URL to undismiss'),
323+
url: z.string().describe('Full GitHub issue or PR URL to undismiss'),
324324
},
325325
annotations: { readOnlyHint: false, destructiveHint: false },
326326
},

0 commit comments

Comments
 (0)