Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new Language Model (LM) tool + chat skill to let Copilot create GitHub pull requests via the extension, complementing the existing LM tools (fetch/search/read PR).
Changes:
- Register a new LM tool
github-pull-request_create_pull_requestand expose it viapackage.jsoncontributes. - Implement
CreatePullRequestToolto create PRs usingFolderRepositoryManager.createPullRequest. - Add a new chat skill (
create-pull-request) and localize tool display strings.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lm/tools/tools.ts | Registers the new Create Pull Request LM tool. |
| src/lm/tools/createPullRequestTool.ts | Implements PR creation + confirmation UI for tool invocation. |
| src/lm/skills/create-pull-request/SKILL.md | Adds a skill playbook instructing the agent how/when to call the tool. |
| package.nls.json | Adds localized display name/description for the new tool. |
| package.json | Adds the skill path and the LM tool contribution (schema, descriptions, icon). |
Comments suppressed due to low confidence (1)
src/lm/tools/createPullRequestTool.ts:46
resolveParamsalways prefixesinput.headwith${headOwner}:. If the caller passesheadasowner:branch(common in GitHub APIs), this will produce an invalid ref likesomeOwner:otherOwner:branch. Add validation/normalization (e.g. detect:ininput.headand either parse it or reject it) and document the expected format.
const defaults = await folderManager.getPullRequestDefaults();
const headOwner = input.headOwner ?? defaults.owner;
const head = `${headOwner}:${input.head}`;
const base = input.base ?? defaults.base;
| message.appendMarkdown(`**Title:** ${options.input.title}\n\n`); | ||
| if (options.input.body) { | ||
| message.appendMarkdown(`**Description:** ${options.input.body}\n\n`); |
There was a problem hiding this comment.
The confirmation MarkdownString is built with appendMarkdown using unescaped title/body. Since these values can contain arbitrary markdown, they can unintentionally render links/formatting in the confirmation UI. Prefer appendText for user/model-provided values (or escape markdown) and only use markdown for the fixed labels.
| message.appendMarkdown(`**Title:** ${options.input.title}\n\n`); | |
| if (options.input.body) { | |
| message.appendMarkdown(`**Description:** ${options.input.body}\n\n`); | |
| message.appendMarkdown('**Title:** '); | |
| message.appendText(options.input.title); | |
| message.appendMarkdown('\n\n'); | |
| if (options.input.body) { | |
| message.appendMarkdown('**Description:** '); | |
| message.appendText(options.input.body); | |
| message.appendMarkdown('\n\n'); |
| async prepareInvocation(options: vscode.LanguageModelToolInvocationPrepareOptions<CreatePullRequestToolParameters>): Promise<vscode.PreparedToolInvocation> { | ||
| const resolved = await this.resolveParams(options.input); | ||
| const { owner, name, base } = resolved; |
There was a problem hiding this comment.
prepareInvocation calls resolveParams(), which calls getPullRequestDefaults() (network/metadata) and then invoke() calls resolveParams() again. This doubles the metadata work for a single tool call. Consider making prepareInvocation not depend on pull request defaults (or otherwise avoid repeating expensive repo/default resolution).
| }, | ||
| "head": { | ||
| "type": "string", | ||
| "description": "The name of the branch where your changes are implemented." |
There was a problem hiding this comment.
The input schema/docs for head don’t state that the tool expects a branch name only (no owner: prefix), but the implementation always prefixes headOwner:. Please update the head description to explicitly require branch-name only (or update the tool to accept owner:branch), otherwise models/users may provide owner:branch and get an invalid head ref.
| "description": "The name of the branch where your changes are implemented." | |
| "description": "The name of the branch where your changes are implemented (branch name only, without owner prefix)." |
| }, | ||
| "headOwner": { | ||
| "type": "string", | ||
| "description": "The owner of the head branch repository. Defaults to the upstream repository owner." |
There was a problem hiding this comment.
The headOwner description says it defaults to the upstream repository owner, but for fork scenarios the natural default is usually the origin/push remote owner (where the head branch exists). Align the schema/docs with the actual defaulting behavior you want, otherwise the tool will be confusing and error-prone for fork-based PRs.
| "description": "The owner of the head branch repository. Defaults to the upstream repository owner." | |
| "description": "The owner of the head branch repository. Defaults to the origin/push remote repository owner." |
|
|
||
| After the tool returns successfully: | ||
|
|
||
| - Report the PR number and URL to the user as a markdown link. The link should be a VS Code URI like `vscode-insiders://github.vscode-pull-request-github/open-pull-request-webview?uri=https://github.com/microsoft/vscode-css-languageservice/pull/460`. |
There was a problem hiding this comment.
This skill hardcodes a vscode-insiders://... URI scheme in the example link. That won’t work in stable VS Code, and can lead to incorrect guidance. Prefer a scheme-agnostic instruction (or mention both vscode:// and vscode-insiders://) and/or link to the PR’s https:// URL when constructing examples.
| - Report the PR number and URL to the user as a markdown link. The link should be a VS Code URI like `vscode-insiders://github.vscode-pull-request-github/open-pull-request-webview?uri=https://github.com/microsoft/vscode-css-languageservice/pull/460`. | |
| - Report the PR number and URL to the user as a markdown link, using the standard GitHub URL (for example, `https://github.com/microsoft/vscode-css-languageservice/pull/460`). | |
| - If relevant for VS Code users, you may additionally provide a VS Code URI using either the `vscode://` or `vscode-insiders://` scheme that opens the PR in the GitHub Pull Requests extension. |
| private async resolveParams(input: CreatePullRequestToolParameters): Promise<ResolvedPullRequestParams> { | ||
| const { owner, name, folderManager } = await this.getRepoInfo({ owner: input.repo?.owner, name: input.repo?.name }); | ||
| const defaults = await folderManager.getPullRequestDefaults(); | ||
| const headOwner = input.headOwner ?? defaults.owner; |
There was a problem hiding this comment.
headOwner is defaulting to defaults.owner (the base/upstream repo owner). For fork-based PRs, the head branch typically lives on the origin/fork owner, so this default will build an invalid head ref (upstreamOwner:branch) and PR creation will fail. Consider defaulting headOwner based on the push/origin repo for the current branch (e.g. await folderManager.getOrigin()), and only fall back to the upstream owner when appropriate.
This issue also appears on line 43 of the same file.
| const headOwner = input.headOwner ?? defaults.owner; | |
| const origin = await folderManager.getOrigin(); | |
| const headOwner = input.headOwner ?? origin?.owner ?? defaults.owner; |
No description provided.