From 9d6e1d305a3909ab4666fa5ead4a5e545584f8a2 Mon Sep 17 00:00:00 2001 From: Jian Weng Date: Wed, 18 Feb 2026 20:28:20 +0300 Subject: [PATCH] [bugfix][#965] Render PR URLs as canonical links in VS Code logs Allow VS Code plan/implementation logs to linkify GitHub PR URLs the same way as issue URLs, while keeping external URL opening restricted to canonical GitHub issue/PR routes. File changes: - `vscode/webview/plan/utils.ts` - Expanded log link detection from issue-only to issue+PR routes. - Tightened matching to canonical route boundaries so extra path/query/fragment segments are not linkified. - `vscode/src/view/unifiedViewProvider.ts` - Expanded URL allowlist from issue-only to issue+PR canonical routes. - Added `openExternalGitHubUrl()` as the single allowlist gate and reused it in: - `plan/view-issue` - `plan/view-pr` - `link/openExternal` - `tests/vscode/test-link-detection.sh` - Updated regex fixtures to match issue+PR canonical behavior. - Added PR-positive detection coverage. - Added allowlist validation coverage for canonical positives and unsupported-route negatives. - `vscode/webview/plan/utils.md` - Documented canonical issue+PR linkification contract for `renderLinks()`. - `vscode/src/view/unifiedViewProvider.md` - Documented canonical issue+PR URL validation and shared external-open gate. - `docs/workflows/vscode-extension.md` - Documented inline issue/PR linkification and canonical external-open restrictions. - `docs/tutorial/03-vscode-planning.md` - Documented clickable canonical issue/PR links in logs. Related issue: - #965 Tests: - `make vscode-plugin`: PASS - `tests/vscode/test-link-detection.sh`: PASS (20 passed, 0 failed) - `TEST_SHELLS="bash zsh" make test-fast`: PASS - Bash suite: 58 passed, 0 failed - Zsh suite: 58 passed, 0 failed - Python suite: 405 passed, 0 failed --- docs/tutorial/03-vscode-planning.md | 2 + docs/workflows/vscode-extension.md | 4 ++ tests/vscode/test-link-detection.sh | 85 ++++++++++++++++++-------- vscode/src/view/unifiedViewProvider.md | 7 ++- vscode/src/view/unifiedViewProvider.ts | 22 ++++--- vscode/webview/plan/utils.md | 6 +- vscode/webview/plan/utils.ts | 2 +- 7 files changed, 88 insertions(+), 40 deletions(-) diff --git a/docs/tutorial/03-vscode-planning.md b/docs/tutorial/03-vscode-planning.md index 6937db00..5e47e187 100644 --- a/docs/tutorial/03-vscode-planning.md +++ b/docs/tutorial/03-vscode-planning.md @@ -50,6 +50,8 @@ Click it to run `lol impl `. The implementation output appears in a separate **Implementation Log** panel, and the button is disabled while the implementation run is active. +Both logs render canonical GitHub issue and PR URLs as clickable links. + ## Next Steps - [Tutorial 02: Issue to Implementation](./02-issue-to-impl.md) for the CLI flow diff --git a/docs/workflows/vscode-extension.md b/docs/workflows/vscode-extension.md index 79a6f077..7038a3ce 100644 --- a/docs/workflows/vscode-extension.md +++ b/docs/workflows/vscode-extension.md @@ -57,6 +57,10 @@ The extension scans stdout/stderr lines in real time and stores the first matchi number on the session. Capturing the issue number during execution ensures the UI can surface the Implement and View PR actions immediately after the plan completes. +Plan and implementation logs also linkify canonical GitHub issue and pull request URLs. +External opening is restricted to canonical `github.com///issues/` and +`github.com///pull/` routes. + When a plan emits a local markdown path (for example, `.tmp/issue-928.md`), the UI surfaces a View Plan button that opens the file inside the workspace. diff --git a/tests/vscode/test-link-detection.sh b/tests/vscode/test-link-detection.sh index d2403533..6f0cfd07 100755 --- a/tests/vscode/test-link-detection.sh +++ b/tests/vscode/test-link-detection.sh @@ -9,17 +9,17 @@ test_info "Testing link detection patterns" TEST_SCRIPT=$(make_temp_dir "link-detection-test")/test.js cat > "$TEST_SCRIPT" << 'NODEJS' -// Regex patterns extracted from vscode/webview/plan/index.ts +// Regex patterns extracted from vscode/webview/plan/utils.ts -// GitHub issue URLs: https://github.com/owner/repo/issues/N -const githubRegex = /https:\/\/github\.com\/([^\/\s]+)\/([^\/\s]+)\/issues\/(\d+)/g; +// Canonical GitHub issue/PR URLs inside log text. +const githubRegex = /https:\/\/github\.com\/[^/\s]+\/[^/\s]+\/(?:issues|pull)\/\d+(?=$|[^\w/?#])/g; // Local markdown paths: .tmp/issue-N.md or /path/to/file.md const mdPathRegex = /(?<=\s|^)(\.tmp\/[^\s\n]+\.md|[\w\-\/]+\.tmp\/[^\s\n]+\.md)(?=\s|$)/g; // isValidGitHubUrl from vscode/src/view/unifiedViewProvider.ts function isValidGitHubUrl(url) { - return /^https:\/\/github\.com\/[^/]+\/[^/]+\/issues\/\d+$/.test(url); + return /^https:\/\/github\.com\/[^/]+\/[^/]+\/(?:issues|pull)\/\d+$/.test(url); } // Test cases for GitHub URL detection @@ -28,42 +28,69 @@ const githubTestCases = [ name: 'Standard GitHub issue URL', input: 'Check https://github.com/owner/repo/issues/123 for details', expectedMatch: ['https://github.com/owner/repo/issues/123'], - expectedValid: true, }, { name: 'GitHub URL with numbers in owner name', input: 'See https://github.com/user123/repo-name/issues/456', expectedMatch: ['https://github.com/user123/repo-name/issues/456'], - expectedValid: true, }, { name: 'Multiple GitHub URLs', input: 'Issues: https://github.com/a/b/issues/1 and https://github.com/c/d/issues/2', expectedMatch: ['https://github.com/a/b/issues/1', 'https://github.com/c/d/issues/2'], - expectedValid: true, }, { - name: 'GitHub PR URL (should not match issue regex)', + name: 'GitHub PR URL', input: 'PR at https://github.com/owner/repo/pull/123', - expectedMatch: [], - expectedValid: false, + expectedMatch: ['https://github.com/owner/repo/pull/123'], }, { name: 'GitHub non-issue URL', input: 'Visit https://github.com/owner/repo', expectedMatch: [], - expectedValid: false, }, { name: 'Non-GitHub URL', input: 'Visit https://example.com/some/path', expectedMatch: [], - expectedValid: false, }, { name: 'Invalid GitHub URL format', input: 'Check https://github.com/owner/issues/123 (no repo)', expectedMatch: [], + }, + { + name: 'GitHub PR URL with extra route segment', + input: 'See https://github.com/owner/repo/pull/123/files', + expectedMatch: [], + }, +]; + +// Test cases for host URL allowlist validation +const githubValidationCases = [ + { + name: 'Canonical issue URL is valid', + url: 'https://github.com/owner/repo/issues/123', + expectedValid: true, + }, + { + name: 'Canonical PR URL is valid', + url: 'https://github.com/owner/repo/pull/456', + expectedValid: true, + }, + { + name: 'Unsupported GitHub route is invalid', + url: 'https://github.com/owner/repo/actions/runs/1', + expectedValid: false, + }, + { + name: 'PR URL with extra segment is invalid', + url: 'https://github.com/owner/repo/pull/123/files', + expectedValid: false, + }, + { + name: 'Non-GitHub URL is invalid', + url: 'https://example.com/owner/repo/issues/1', expectedValid: false, }, ]; @@ -114,25 +141,31 @@ let failed = 0; console.log('Testing GitHub URL detection:\n'); githubTestCases.forEach(tc => { const matches = [...tc.input.matchAll(githubRegex)].map(m => m[0]); - const isValid = tc.expectedMatch.length > 0 ? isValidGitHubUrl(tc.expectedMatch[0]) : false; - - let matchOk = JSON.stringify(matches) === JSON.stringify(tc.expectedMatch); - let validOk = isValid === tc.expectedValid; - - if (matchOk && validOk) { + + if (JSON.stringify(matches) === JSON.stringify(tc.expectedMatch)) { console.log(`✓ PASS: ${tc.name}`); passed++; } else { console.log(`✗ FAIL: ${tc.name}`); console.log(` Input: ${tc.input}`); - if (!matchOk) { - console.log(` Expected matches: ${JSON.stringify(tc.expectedMatch)}`); - console.log(` Got matches: ${JSON.stringify(matches)}`); - } - if (!validOk) { - console.log(` Expected valid: ${tc.expectedValid}`); - console.log(` Got valid: ${isValid}`); - } + console.log(` Expected matches: ${JSON.stringify(tc.expectedMatch)}`); + console.log(` Got matches: ${JSON.stringify(matches)}`); + failed++; + } +}); + +// Test URL allowlist validation +console.log('\nTesting GitHub URL allowlist validation:\n'); +githubValidationCases.forEach(tc => { + const actual = isValidGitHubUrl(tc.url); + if (actual === tc.expectedValid) { + console.log(`✓ PASS: ${tc.name}`); + passed++; + } else { + console.log(`✗ FAIL: ${tc.name}`); + console.log(` URL: ${tc.url}`); + console.log(` Expected valid: ${tc.expectedValid}`); + console.log(` Got valid: ${actual}`); failed++; } }); diff --git a/vscode/src/view/unifiedViewProvider.md b/vscode/src/view/unifiedViewProvider.md index f0c99b3d..898aef53 100644 --- a/vscode/src/view/unifiedViewProvider.md +++ b/vscode/src/view/unifiedViewProvider.md @@ -26,7 +26,7 @@ Consumes UI messages: - `plan/view-plan` - `plan/view-issue` - `plan/view-pr` -- `link/openExternal` (GitHub issue URLs) +- `link/openExternal` (canonical GitHub issue/PR URLs) - `link/openFile` (local markdown paths) Emits UI messages: @@ -111,7 +111,10 @@ Resolves the working directory for Plan/Implementation runs by preferring the `trees/main` layout and falling back to an Agentize worktree root when needed. ### Link Handling -- `isValidGitHubUrl(url: string)`: validates GitHub issue URLs. +- `isValidGitHubUrl(url: string)`: validates canonical GitHub issue/PR URLs + (`/issues/` or `/pull/`). +- `openExternalGitHubUrl(url: string)`: central allowlist gate for external URL open + paths (`plan/view-issue`, `plan/view-pr`, and `link/openExternal`). - `openLocalFile(filePath: string, options?)`: resolves relative paths from workspace roots, expands `~/...`, optionally creates missing files, and opens in the current editor group as a non-preview tab. diff --git a/vscode/src/view/unifiedViewProvider.ts b/vscode/src/view/unifiedViewProvider.ts index aef53832..2db31db3 100644 --- a/vscode/src/view/unifiedViewProvider.ts +++ b/vscode/src/view/unifiedViewProvider.ts @@ -462,8 +462,8 @@ export class UnifiedViewProvider implements vscode.WebviewViewProvider { return; } const issueUrl = await this.resolveIssueUrl(issueNumber); - if (issueUrl && this.isValidGitHubUrl(issueUrl)) { - void vscode.env.openExternal(vscode.Uri.parse(issueUrl)); + if (issueUrl) { + this.openExternalGitHubUrl(issueUrl); } return; } @@ -476,7 +476,7 @@ export class UnifiedViewProvider implements vscode.WebviewViewProvider { if (!session || !session.prUrl) { return; } - void vscode.env.openExternal(vscode.Uri.parse(session.prUrl)); + this.openExternalGitHubUrl(session.prUrl); return; } case 'settings/load': { @@ -488,10 +488,7 @@ export class UnifiedViewProvider implements vscode.WebviewViewProvider { return; } case 'link/openExternal': { - const url = message.url ?? ''; - if (this.isValidGitHubUrl(url)) { - void vscode.env.openExternal(vscode.Uri.parse(url)); - } + this.openExternalGitHubUrl(message.url ?? ''); return; } case 'link/openFile': { @@ -508,9 +505,16 @@ export class UnifiedViewProvider implements vscode.WebviewViewProvider { } } + private openExternalGitHubUrl(url: string): void { + if (!this.isValidGitHubUrl(url)) { + return; + } + void vscode.env.openExternal(vscode.Uri.parse(url)); + } + private isValidGitHubUrl(url: string): boolean { - // Validate GitHub issue URLs: https://github.com/owner/repo/issues/N - return /^https:\/\/github\.com\/[^/]+\/[^/]+\/issues\/\d+$/.test(url); + // Allow only canonical GitHub issue/PR URLs. + return /^https:\/\/github\.com\/[^/]+\/[^/]+\/(?:issues|pull)\/\d+$/.test(url); } private async openLocalFile( diff --git a/vscode/webview/plan/utils.md b/vscode/webview/plan/utils.md index 319044a0..b4fe7106 100644 --- a/vscode/webview/plan/utils.md +++ b/vscode/webview/plan/utils.md @@ -45,8 +45,10 @@ Builds and returns a DOM container with step indicators for the given session ke Uses `className` when provided; defaults to `step-indicators`. ### renderLinks(text) -Converts GitHub issue URLs and local `.tmp/*.md` paths in a log line into clickable links -for the webview message handlers. +Converts canonical GitHub issue/PR URLs and local `.tmp/*.md` paths in a log line into +clickable links for the webview message handlers. Only canonical routes are linkified: +`https://github.com///issues/` and +`https://github.com///pull/`. ### extractIssueNumber(line) Extracts an issue number from placeholder creation logs or GitHub issue URLs. diff --git a/vscode/webview/plan/utils.ts b/vscode/webview/plan/utils.ts index 4e24a4bd..79d74657 100644 --- a/vscode/webview/plan/utils.ts +++ b/vscode/webview/plan/utils.ts @@ -124,7 +124,7 @@ export const renderStepIndicatorsFrom = ( }; export const renderLinks = (text: string): string => { - const githubRegex = /https:\/\/github\.com\/([^\/\s]+)\/([^\/\s]+)\/issues\/(\d+)/g; + const githubRegex = /https:\/\/github\.com\/[^/\s]+\/[^/\s]+\/(?:issues|pull)\/\d+(?=$|[^\w/?#])/g; const mdPathRegex = /(?<=\s|^)(\.tmp\/[^\s\n]+\.md|[\w\-\/]+\.tmp\/[^\s\n]+\.md)(?=\s|$)/g; let result = escapeHtml(text);