diff --git a/package.json b/package.json index 388c22181e..ad77aa049f 100644 --- a/package.json +++ b/package.json @@ -320,6 +320,14 @@ "default": false, "description": "%githubPullRequests.deleteBranchAfterMerge.description%" }, + "githubPullRequests.enableAttestationCommits": { + "type": [ + "boolean", + "string" + ], + "default": false, + "markdownDescription": "%githubPullRequests.enableAttestationCommits.description%" + }, "githubPullRequests.terminalLinksHandler": { "type": "string", "enum": [ @@ -1546,6 +1554,11 @@ "title": "%command.review.approveOnDotCom.title%", "category": "%command.pull.request.category%" }, + { + "command": "pr.addAttestationCommit", + "title": "%command.pr.addAttestationCommit.title%", + "category": "%command.pull.request.category%" + }, { "command": "review.requestChangesOnDotCom", "title": "%command.review.requestChangesOnDotCom.title%", @@ -2084,6 +2097,10 @@ "command": "github.api.preloadPullRequest", "when": "false" }, + { + "command": "pr.addAttestationCommit", + "when": "config.githubPullRequests.enableAttestationCommits" + }, { "command": "pr.configureRemotes", "when": "gitHubOpenRepositoryCount != 0" diff --git a/package.nls.json b/package.nls.json index ed80e9bbf5..ea64843094 100644 --- a/package.nls.json +++ b/package.nls.json @@ -46,6 +46,7 @@ "githubPullRequests.defaultDeletionMethod.selectRemote.description": "When true, the option to delete the remote will be selected by default when deleting a branch from a pull request.", "githubPullRequests.defaultDeletionMethod.selectWorktree.description": "When true, the option to remove the associated worktree will be selected by default when deleting a branch from a pull request.", "githubPullRequests.deleteBranchAfterMerge.description": "Automatically delete the branch after merging a pull request. This setting only applies when the pull request is merged through this extension. When using merge queues, this will only delete the local branch.", + "githubPullRequests.enableAttestationCommits.description": "Enables adding an attestation commit (an empty, signed commit) to the head of a pull request branch as a way to attest to a pull request even when its individual commits are unsigned. Requires commit signing to be configured for git. Set to `true` to enable with the default commit message, set to a string to use that string as the commit message, or set to `false` to disable.", "githubPullRequests.terminalLinksHandler.description": "Default handler for terminal links.", "githubPullRequests.terminalLinksHandler.github": "Create the pull request on GitHub.", "githubPullRequests.terminalLinksHandler.vscode": "Create the pull request in VS Code.", @@ -246,6 +247,7 @@ "command.review.comment.title": "Comment", "command.review.requestChanges.title": "Request Changes", "command.review.approveOnDotCom.title": "Approve on github.com", + "command.pr.addAttestationCommit.title": "Add Attestation Commit", "command.review.requestChangesOnDotCom.title": "Request changes on github.com", "command.review.createSuggestionsFromChanges.title": "Create Pull Request Suggestions", "command.review.createSuggestionFromChange.title": "Convert to Pull Request Suggestion", diff --git a/src/commands.ts b/src/commands.ts index 0240cce83a..79f4303e59 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -18,6 +18,7 @@ import { SessionLinkInfo } from './common/timelineEvent'; import { asTempStorageURI, fromPRUri, fromReviewUri, Schemes, toPRUri } from './common/uri'; import { formatError } from './common/utils'; import { EXTENSION_ID } from './constants'; +import { addAttestationCommit } from './github/attestationCommit'; import { CrossChatSessionWithPR } from './github/copilotApi'; import { CopilotRemoteAgentManager, SessionIdForPr } from './github/copilotRemoteAgent'; import { guessExtensionFromMime, pickFilesForUpload, placeholdersForNames, runFileUploads, runPendingUploads } from './github/fileUpload'; @@ -218,6 +219,51 @@ export function registerCommands( ), ); + context.subscriptions.push( + vscode.commands.registerCommand( + 'pr.addAttestationCommit', + async (target?: PullRequestModel | PRNode | RepositoryChangesNode) => { + let pr: PullRequestModel | undefined; + let folderManager: FolderRepositoryManager | undefined; + + if (target instanceof PullRequestModel) { + pr = target; + } else if (target && (target as PRNode).pullRequestModel) { + pr = (target as PRNode).pullRequestModel; + } else if (target && (target as RepositoryChangesNode).pullRequestModel) { + pr = (target as RepositoryChangesNode).pullRequestModel; + } + + if (pr) { + folderManager = reposManager.getManagerForIssueModel(pr) ?? reposManager.folderManagers.find(m => m.activePullRequest?.equals(pr!)); + } + + if (!pr || !folderManager) { + const activePullRequestsWithFolderManager = reposManager.folderManagers + .filter(m => m.activePullRequest) + .map(m => ({ activePr: m.activePullRequest!, folderManager: m })); + + if (activePullRequestsWithFolderManager.length === 0) { + vscode.window.showErrorMessage(vscode.l10n.t('No active pull request to add an attestation commit to.')); + return; + } + + const picked = activePullRequestsWithFolderManager.length === 1 + ? activePullRequestsWithFolderManager[0] + : await chooseItem(activePullRequestsWithFolderManager, item => ({ label: item.activePr.html_url })); + + if (!picked) { + return; + } + pr = picked.activePr; + folderManager = picked.folderManager; + } + + await addAttestationCommit(folderManager, pr); + }, + ), + ); + context.subscriptions.push( vscode.commands.registerCommand('pr.openFileOnGitHub', async (e: GitFileChangeNode | RemoteFileChangeNode) => { if (e instanceof RemoteFileChangeNode) { diff --git a/src/common/settingKeys.ts b/src/common/settingKeys.ts index ba0654cc23..0f5e64399b 100644 --- a/src/common/settingKeys.ts +++ b/src/common/settingKeys.ts @@ -38,6 +38,7 @@ export const SELECT_LOCAL_BRANCH = 'selectLocalBranch'; export const SELECT_REMOTE = 'selectRemote'; export const SELECT_WORKTREE = 'selectWorktree'; export const DELETE_BRANCH_AFTER_MERGE = 'deleteBranchAfterMerge'; +export const ENABLE_ATTESTATION_COMMITS = 'enableAttestationCommits'; export const REMOTES = 'remotes'; export const PULL_PR_BRANCH_BEFORE_CHECKOUT = 'pullPullRequestBranchBeforeCheckout'; export type PullPRBranchVariants = 'never' | 'pull' | 'pullAndMergeBase' | 'pullAndUpdateBase' | true | false; diff --git a/src/github/activityBarViewProvider.ts b/src/github/activityBarViewProvider.ts index 65deb964fa..72cc4ccc8f 100644 --- a/src/github/activityBarViewProvider.ts +++ b/src/github/activityBarViewProvider.ts @@ -5,13 +5,14 @@ import * as vscode from 'vscode'; import { openPullRequestOnGitHub } from '../commands'; +import { addAttestationCommit, isAttestationCommitsEnabled } from './attestationCommit'; import { FolderRepositoryManager } from './folderRepositoryManager'; import { GithubItemStateEnum, IAccount, MergeMethod, ReviewEventEnum, ReviewState } from './interface'; import { isCopilotOnMyBehalf, PullRequestModel } from './pullRequestModel'; import { getDefaultMergeMethod } from './pullRequestOverview'; import { PullRequestReviewCommon, ReviewContext } from './pullRequestReviewCommon'; import { isInCodespaces, parseReviewers } from './utils'; -import { MergeArguments, PullRequest, ReviewType } from './views'; +import { MergeArguments, PullRequest, ReviewType, SubmitReviewArgs } from './views'; import { IComment } from '../common/comment'; import { emojify, ensureEmojis } from '../common/emoji'; import { disposeAll } from '../common/lifecycle'; @@ -108,6 +109,8 @@ export class PullRequestViewProvider extends WebviewViewBase implements vscode.W return this.updateBranch(message); case 'pr.re-request-review': return this.reRequestReview(message); + case 'pr.add-attestation-commit': + return this.addAttestationCommitMessage(message); } } @@ -119,6 +122,11 @@ export class PullRequestViewProvider extends WebviewViewBase implements vscode.W return PullRequestReviewCommon.reRequestReview(this.getReviewContext(), message); } + private async addAttestationCommitMessage(message: IRequestMessage): Promise { + const sha = await addAttestationCommit(this._folderRepositoryManager, this._item); + this._replyMessage(message, { success: !!sha, sha }); + } + public async refresh(): Promise { return vscode.window.withProgress({ location: { viewId: 'github:activePullRequest' } }, async () => { await this._item.initializeReviewThreadCache(); @@ -302,7 +310,8 @@ export class PullRequestViewProvider extends WebviewViewBase implements vscode.W isEnterprise: pullRequest.githubRepository.remote.isEnterprise, hasReviewDraft, currentUserReviewState: reviewState, - isCopilotOnMyBehalf: await isCopilotOnMyBehalf(pullRequest, currentUser, coAuthors) + isCopilotOnMyBehalf: await isCopilotOnMyBehalf(pullRequest, currentUser, coAuthors), + attestationCommitsEnabled: isAttestationCommitsEnabled() }; this._postMessage({ @@ -348,21 +357,43 @@ export class PullRequestViewProvider extends WebviewViewBase implements vscode.W ); } - private async doReviewMessage(message: IRequestMessage, action: (body) => Promise) { + private async doReviewMessage(message: IRequestMessage, action: (body) => Promise, needsTimelineRefresh: boolean = false) { return PullRequestReviewCommon.doReviewMessage( this.getReviewContext(), message, - false, - action + needsTimelineRefresh, + action, ); } + /** + * Handles a review-submission message that may include a request to first + * push a signed attestation commit. If attestation fails, the review is + * NOT submitted. + */ + private async doReviewMessageWithAttestation( + message: IRequestMessage, + action: (body: string) => Promise, + ): Promise { + const { body, addAttestation } = message.args ?? { body: '' }; + let attestationSha: string | undefined; + if (addAttestation) { + attestationSha = await addAttestationCommit(this._folderRepositoryManager, this._item); + if (!attestationSha) { + this._throwError(message, 'Attestation commit failed; review was not submitted.'); + return; + } + } + const forwarded: IRequestMessage = { req: message.req, command: message.command, args: body }; + await this.doReviewMessage(forwarded, action, !!attestationSha); + } + private approvePullRequest(body: string): Promise { return this._item.approve(this._folderRepositoryManager.repository, body); } - private async approvePullRequestMessage(message: IRequestMessage): Promise { - await this.doReviewMessage(message, (body) => this.approvePullRequest(body)); + private async approvePullRequestMessage(message: IRequestMessage): Promise { + await this.doReviewMessageWithAttestation(message, (body) => this.approvePullRequest(body)); } private async approvePullRequestCommand(context: { body: string }): Promise { @@ -377,8 +408,8 @@ export class PullRequestViewProvider extends WebviewViewBase implements vscode.W await this.doReviewCommand(context, ReviewType.RequestChanges, (body) => this.requestChanges(body)); } - private async requestChangesMessage(message: IRequestMessage): Promise { - await this.doReviewMessage(message, (body) => this.requestChanges(body)); + private async requestChangesMessage(message: IRequestMessage): Promise { + await this.doReviewMessageWithAttestation(message, (body) => this.requestChanges(body)); } private submitReview(body: string): Promise { @@ -389,8 +420,8 @@ export class PullRequestViewProvider extends WebviewViewBase implements vscode.W return this.doReviewCommand(context, ReviewType.Comment, (body) => this.submitReview(body)); } - private submitReviewMessage(message: IRequestMessage) { - return this.doReviewMessage(message, (body) => this.submitReview(body)); + private submitReviewMessage(message: IRequestMessage) { + return this.doReviewMessageWithAttestation(message, (body) => this.submitReview(body)); } private async deleteBranch(message: IRequestMessage) { diff --git a/src/github/attestationCommit.ts b/src/github/attestationCommit.ts new file mode 100644 index 0000000000..8ee9993055 --- /dev/null +++ b/src/github/attestationCommit.ts @@ -0,0 +1,142 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import * as vscode from 'vscode'; +import { FolderRepositoryManager } from './folderRepositoryManager'; +import { PullRequestModel } from './pullRequestModel'; +import { Repository } from '../api/api'; +import Logger from '../common/logger'; +import { ENABLE_ATTESTATION_COMMITS, PR_SETTINGS_NAMESPACE } from '../common/settingKeys'; +import { formatError } from '../common/utils'; + +const LOG_ID = 'AttestationCommit'; + +const DEFAULT_ATTESTATION_COMMIT_MESSAGE = 'Attestation commit'; + +/** + * Returns true when the repository appears to have commit signing configured. + * + * Accepts any of the following (checked across local + global git config): + * - `user.signingkey` is set, OR + * - `commit.gpgsign` is `true` (git will pick a default signing identity), OR + * - `gpg.format` is set to `ssh` or `x509` (the user is explicitly opting in + * to a non-default signing format). + */ +async function hasCommitSigningConfigured(repository: Repository): Promise { + const read = async (key: string): Promise => { + const tryRead = async (fn: (k: string) => Promise): Promise => { + try { + const value = await fn(key); + return value?.trim() || undefined; + } catch { + // `getConfig`/`getGlobalConfig` reject when the key is not set. + return undefined; + } + }; + return (await tryRead(k => repository.getConfig(k))) + ?? (await tryRead(k => repository.getGlobalConfig(k))); + }; + + const [signingKey, commitGpgSign, gpgFormat] = await Promise.all([ + read('user.signingkey'), + read('commit.gpgsign'), + read('gpg.format'), + ]); + + if (signingKey) { + return true; + } + if (commitGpgSign?.toLowerCase() === 'true') { + return true; + } + if (gpgFormat && ['ssh', 'x509'].includes(gpgFormat.toLowerCase())) { + return true; + } + return false; +} + +/** + * Reads the `githubPullRequests.enableAttestationCommits` setting. + * Returns `false` when the feature is disabled, otherwise the commit message + * that should be used for the attestation commit. + */ +export function getAttestationCommitSetting(): false | string { + const value = vscode.workspace + .getConfiguration(PR_SETTINGS_NAMESPACE) + .get(ENABLE_ATTESTATION_COMMITS, false); + + if (typeof value === 'string') { + const trimmed = value.trim(); + if (trimmed.length === 0) { + return false; + } + return trimmed; + } + return value === true ? DEFAULT_ATTESTATION_COMMIT_MESSAGE : false; +} + +/** + * Whether the attestation commit feature is enabled by the user setting. + */ +export function isAttestationCommitsEnabled(): boolean { + return getAttestationCommitSetting() !== false; +} + +/** + * Adds an empty, signed "attestation" commit to the head of the given pull request branch + * and pushes it to the corresponding remote. Requires that the pull request is currently + * checked out and that the user has commit signing configured. + * + * Returns the SHA of the new attestation commit when successful, otherwise `undefined`. + */ +export async function addAttestationCommit( + folderRepositoryManager: FolderRepositoryManager, + pullRequestModel: PullRequestModel, +): Promise { + const message = getAttestationCommitSetting(); + if (message === false) { + vscode.window.showWarningMessage(vscode.l10n.t('Attestation commits are not enabled. Enable them via the `githubPullRequests.enableAttestationCommits` setting.')); + return undefined; + } + + const activePullRequest = folderRepositoryManager.activePullRequest; + if (!activePullRequest || !activePullRequest.equals(pullRequestModel)) { + vscode.window.showErrorMessage(vscode.l10n.t('The pull request must be checked out before an attestation commit can be added.')); + return undefined; + } + + const repository = folderRepositoryManager.repository; + const head = repository.state.HEAD; + if (!head || !head.name) { + vscode.window.showErrorMessage(vscode.l10n.t('Unable to add an attestation commit: no branch is currently checked out.')); + return undefined; + } + + if (!await hasCommitSigningConfigured(repository)) { + vscode.window.showErrorMessage(vscode.l10n.t('Unable to add an attestation commit: commit signing does not appear to be configured. Set `user.signingkey` (or enable `commit.gpgsign`) in your git config and ensure your signing tool (GPG, SSH, or X.509) is set up.')); + return undefined; + } + + try { + Logger.appendLine(`Creating attestation commit on branch ${head.name} for PR #${pullRequestModel.number}`, LOG_ID); + await repository.commit(message, { + empty: true, + signCommit: true, + }); + + const upstream = head.upstream; + if (upstream) { + await repository.push(upstream.remote, head.name); + } else { + await repository.push(); + } + + return repository.state.HEAD?.commit; + } catch (e) { + Logger.error(`Failed to add attestation commit: ${formatError(e)}`, LOG_ID); + vscode.window.showErrorMessage(vscode.l10n.t('Failed to add attestation commit: {0}', formatError(e))); + return undefined; + } +} diff --git a/src/github/issueOverview.ts b/src/github/issueOverview.ts index 959276e232..30e588ee2b 100644 --- a/src/github/issueOverview.ts +++ b/src/github/issueOverview.ts @@ -13,7 +13,7 @@ import { GithubItemStateEnum, IAccount, IMilestone, IProject, IProjectItem, Repo import { IssueModel } from './issueModel'; import { getAssigneesQuickPickItems, getLabelOptions, getMilestoneFromQuickPick, getProjectFromQuickPick } from './quickPicks'; import { isInCodespaces, processPermalinks, vscodeDevPrLink } from './utils'; -import { ChangeAssigneesReply, DisplayLabel, FileUploadCompletedMessage, Issue, ProjectItemsReply, SubmitReviewReply, UnresolvedIdentity, UploadFilesReply, UploadPastedFilesArgs } from './views'; +import { ChangeAssigneesReply, DisplayLabel, FileUploadCompletedMessage, Issue, ProjectItemsReply, SubmitReviewArgs, SubmitReviewReply, UnresolvedIdentity, UploadFilesReply, UploadPastedFilesArgs } from './views'; import { COPILOT_ACCOUNTS, IComment } from '../common/comment'; import { emojify, ensureEmojis } from '../common/emoji'; import Logger from '../common/logger'; @@ -462,8 +462,9 @@ export class IssueOverviewPanel extends W } } - protected async submitReviewMessage(message: IRequestMessage) { - const comment = await this._item.createIssueComment(message.args); + protected async submitReviewMessage(message: IRequestMessage) { + const body = message.args?.body ?? ''; + const comment = await this._item.createIssueComment(body); const commentedEvent: CommentEvent = { ...comment, event: EventType.Commented diff --git a/src/github/pullRequestOverview.ts b/src/github/pullRequestOverview.ts index 77e14c3709..6d789e6b0b 100644 --- a/src/github/pullRequestOverview.ts +++ b/src/github/pullRequestOverview.ts @@ -8,6 +8,7 @@ import * as crypto from 'crypto'; import * as vscode from 'vscode'; import { OpenCommitChangesArgs, OpenLocalFileArgs } from '../../common/views'; import { openPullRequestOnGitHub } from '../commands'; +import { addAttestationCommit, isAttestationCommitsEnabled } from './attestationCommit'; import { getCopilotApi } from './copilotApi'; import { SessionIdForPr } from './copilotRemoteAgent'; import { FolderRepositoryManager } from './folderRepositoryManager'; @@ -28,7 +29,7 @@ import { isCopilotOnMyBehalf, PullRequestModel } from './pullRequestModel'; import { PullRequestReviewCommon, ReviewContext } from './pullRequestReviewCommon'; import { branchPicks, pickEmail, reviewersQuickPick } from './quickPicks'; import { getEnterpriseUri, getIssueOrURLExpression, parseIssueExpressionOutput, parseReviewers, processDiffLinks, processPermalinks } from './utils'; -import { CancelCodingAgentReply, ChangeBaseReply, ChangeReviewersReply, DeleteReviewResult, MergeArguments, MergeResult, PullRequest, ReadyForReviewAndMergeContext, ReadyForReviewContext, ReviewCommentContext, ReviewType, UnresolvedIdentity } from './views'; +import { CancelCodingAgentReply, ChangeBaseReply, ChangeReviewersReply, DeleteReviewResult, MergeArguments, MergeResult, PullRequest, ReadyForReviewAndMergeContext, ReadyForReviewContext, ReviewCommentContext, ReviewType, SubmitReviewArgs, UnresolvedIdentity } from './views'; import { debounce } from '../common/async'; import { COPILOT_ACCOUNTS, IComment } from '../common/comment'; import { COPILOT_REVIEWER, COPILOT_REVIEWER_ACCOUNT, COPILOT_SWE_AGENT, copilotEventToStatus, CopilotPRStatus, mostRecentCopilotEvent } from '../common/copilot'; @@ -409,7 +410,7 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel COPILOT_ACCOUNTS[user.login]); const isCopilotAlreadyReviewer = this._existingReviewers.some(reviewer => !isITeam(reviewer.reviewer) && reviewer.reviewer.login === COPILOT_REVIEWER); - const baseContext = await this.getInitializeContext(currentUser, pullRequest, timelineEvents, repositoryAccess, viewerCanEdit, users); + const baseContext = await this.getInitializeContext(currentUser, pullRequest, timelineEvents ?? [], repositoryAccess, viewerCanEdit, users); this.preLoadInfoNotRequiredForOverview(pullRequest); @@ -460,6 +461,7 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel { const enterpriseUri = pullRequest.remote.isEnterprise ? getEnterpriseUri() : undefined; const issueOrUrlExpression = getIssueOrURLExpression(enterpriseUri); @@ -583,9 +585,16 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel): Promise { + const sha = await addAttestationCommit(this._folderRepositoryManager, this._item); + this._replyMessage(message, { success: !!sha, sha }); + } + private gotoChangesSinceReview(message: IRequestMessage): Promise { if (!this._item.showChangesSinceReview) { this._item.showChangesSinceReview = true; @@ -936,12 +945,34 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel, + action: (body: string) => Promise, + ): Promise { + const { body, addAttestation } = message.args ?? { body: '' }; + let attestationSha: string | undefined; + if (addAttestation) { + attestationSha = await addAttestationCommit(this._folderRepositoryManager, this._item); + if (!attestationSha) { + this._throwError(message, 'Attestation commit failed; review was not submitted.'); + return; + } + } + const forwarded: IRequestMessage = { req: message.req, command: message.command, args: body }; + await this.doReviewMessage(forwarded, action); + } + private approvePullRequest(body: string): Promise { return this._item.approve(this._folderRepositoryManager.repository, body); } - private approvePullRequestMessage(message: IRequestMessage): Promise { - return this.doReviewMessage(message, (body) => this.approvePullRequest(body)); + private approvePullRequestMessage(message: IRequestMessage): Promise { + return this.doReviewMessageWithAttestation(message, (body) => this.approvePullRequest(body)); } private approvePullRequestCommand(context: { body: string }): Promise { @@ -956,8 +987,8 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel this.requestChanges(body)); } - private requestChangesMessage(message: IRequestMessage): Promise { - return this.doReviewMessage(message, (body) => this.requestChanges(body)); + private requestChangesMessage(message: IRequestMessage): Promise { + return this.doReviewMessageWithAttestation(message, (body) => this.requestChanges(body)); } private submitReview(body: string): Promise { @@ -968,8 +999,8 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel this.submitReview(body)); } - protected override submitReviewMessage(message: IRequestMessage) { - return this.doReviewMessage(message, (body) => this.submitReview(body)); + protected override submitReviewMessage(message: IRequestMessage) { + return this.doReviewMessageWithAttestation(message, (body) => this.submitReview(body)); } private reRequestReview(message: IRequestMessage): void { diff --git a/src/github/utils.ts b/src/github/utils.ts index 5b69998881..23de230b2f 100644 --- a/src/github/utils.ts +++ b/src/github/utils.ts @@ -1700,6 +1700,7 @@ export function insertNewCommitsSinceReview( ) { if (latestReviewCommitOid && head && head.sha !== latestReviewCommitOid) { let lastViewerReviewIndex: number = timelineEvents.length - 1; + let lastViewerReviewTime: number | undefined; let comittedDuringReview: boolean = false; let interReviewCommits: Common.TimelineEvent[] = []; @@ -1716,8 +1717,21 @@ export function insertNewCommitsSinceReview( break; } else if (comittedDuringReview && timelineEvents[i].event === Common.EventType.Committed) { - interReviewCommits.unshift(timelineEvents[i]); - timelineEvents.splice(i, 1); + // Only treat a commit as "new since review" when it was actually + // committed AFTER the user's most recent review. Otherwise (e.g. + // for an attestation commit pushed just before the review) leave + // it in its original chronological position. + const committedDate = (timelineEvents[i] as Common.CommitEvent).committedDate; + const committedTime = committedDate ? new Date(committedDate).getTime() : undefined; + if (lastViewerReviewTime === undefined || committedTime === undefined || committedTime > lastViewerReviewTime) { + interReviewCommits.unshift(timelineEvents[i]); + timelineEvents.splice(i, 1); + if (i <= lastViewerReviewIndex) { + // Keep `lastViewerReviewIndex` pointing at the same review + // after the array shrinks. + lastViewerReviewIndex--; + } + } } else if ( !comittedDuringReview && @@ -1725,6 +1739,8 @@ export function insertNewCommitsSinceReview( (timelineEvents[i] as Common.ReviewEvent).user.login === currentUser ) { lastViewerReviewIndex = i; + const submittedAt = (timelineEvents[i] as Common.ReviewEvent).submittedAt; + lastViewerReviewTime = submittedAt ? new Date(submittedAt).getTime() : undefined; comittedDuringReview = true; } } diff --git a/src/github/views.ts b/src/github/views.ts index 6a84b56835..4dd3108f4b 100644 --- a/src/github/views.ts +++ b/src/github/views.ts @@ -114,6 +114,7 @@ export interface PullRequest extends Issue { loadingCommit?: string; generateDescriptionTitle?: string; closingIssues?: IssueReference[]; + attestationCommitsEnabled?: boolean; } export interface ProjectItemsReply { @@ -135,6 +136,11 @@ export interface SubmitReviewReply { reviewers?: ReviewState[]; } +export interface SubmitReviewArgs { + body: string; + addAttestation?: boolean; +} + export interface ReadyForReviewReply { isDraft: boolean; reviewEvent?: ReviewEvent; diff --git a/src/test/github/utils.test.ts b/src/test/github/utils.test.ts index 12db79f4cb..02f068550e 100644 --- a/src/test/github/utils.test.ts +++ b/src/test/github/utils.test.ts @@ -4,8 +4,11 @@ *--------------------------------------------------------------------------------------------*/ import { default as assert } from 'assert'; -import { getPRFetchQuery, sanitizeIssueTitle, variableSubstitution } from '../../github/utils'; +import { AccountType } from '../../github/interface'; +import { getPRFetchQuery, insertNewCommitsSinceReview, sanitizeIssueTitle, variableSubstitution } from '../../github/utils'; import { IssueModel } from '../../github/issueModel'; +import { GitHubRef } from '../../common/githubRef'; +import { CommitEvent, EventType, ReviewEvent, TimelineEvent } from '../../common/timelineEvent'; describe('utils', () => { @@ -76,4 +79,111 @@ describe('utils', () => { assert.strictEqual(result, '${issueType}-7'); }); }); + + describe('insertNewCommitsSinceReview', () => { + const CURRENT_USER = 'octocat'; + const LATEST_REVIEW_SHA = 'shaA'; + const HEAD_SHA = 'shaC'; + + function makeHead(sha: string): GitHubRef { + return new GitHubRef('refs/heads/feature', 'octocat:feature', sha, 'https://github.com/octocat/repo.git', 'octocat', 'repo', false); + } + + function makeCommit(sha: string, committedDate: Date): CommitEvent { + return { + id: sha, + event: EventType.Committed, + sha, + htmlUrl: `https://github.com/octocat/repo/commit/${sha}`, + message: `commit ${sha}`, + committedDate, + author: { + login: CURRENT_USER, + id: '1', + url: 'https://github.com/octocat', + accountType: AccountType.User, + }, + }; + } + + function makeReview(submittedAt: string, login: string = CURRENT_USER): ReviewEvent { + return { + id: 1, + event: EventType.Reviewed, + comments: [], + submittedAt, + body: '', + htmlUrl: '', + authorAssociation: 'OWNER', + user: { + login, + id: '1', + url: `https://github.com/${login}`, + accountType: AccountType.User, + }, + }; + } + + it('moves a commit pushed AFTER the user\'s review under NewCommitsSinceReview', () => { + const reviewTime = new Date('2024-01-01T12:00:00Z'); + const events: TimelineEvent[] = [ + makeCommit(LATEST_REVIEW_SHA, new Date('2024-01-01T11:00:00Z')), + makeReview(reviewTime.toISOString()), + makeCommit(HEAD_SHA, new Date('2024-01-01T13:00:00Z')), + ]; + + insertNewCommitsSinceReview(events, LATEST_REVIEW_SHA, CURRENT_USER, makeHead(HEAD_SHA)); + + // Expected order: latest-review commit, review, NewCommitsSinceReview marker, post-review commit + assert.strictEqual(events.length, 4); + assert.strictEqual(events[0].event, EventType.Committed); + assert.strictEqual((events[0] as CommitEvent).sha, LATEST_REVIEW_SHA); + assert.strictEqual(events[1].event, EventType.Reviewed); + assert.strictEqual(events[2].event, EventType.NewCommitsSinceReview); + assert.strictEqual(events[3].event, EventType.Committed); + assert.strictEqual((events[3] as CommitEvent).sha, HEAD_SHA); + }); + + it('does NOT move a commit pushed just BEFORE the user\'s review (e.g. an attestation commit)', () => { + const reviewTime = new Date('2024-01-01T12:00:00Z'); + const attestationCommitTime = new Date('2024-01-01T11:59:00Z'); // 1 minute before the review + const events: TimelineEvent[] = [ + makeCommit(LATEST_REVIEW_SHA, new Date('2024-01-01T10:00:00Z')), + makeCommit(HEAD_SHA, attestationCommitTime), + makeReview(reviewTime.toISOString()), + ]; + + insertNewCommitsSinceReview(events, LATEST_REVIEW_SHA, CURRENT_USER, makeHead(HEAD_SHA)); + + // Expected: pre-review commit stays in place (not under NewCommitsSinceReview), + // and the marker is still inserted after the review for any later commits (none here). + assert.strictEqual(events.length, 4); + assert.strictEqual(events[0].event, EventType.Committed); + assert.strictEqual((events[0] as CommitEvent).sha, LATEST_REVIEW_SHA); + assert.strictEqual(events[1].event, EventType.NewCommitsSinceReview); + assert.strictEqual(events[2].event, EventType.Committed); + assert.strictEqual((events[2] as CommitEvent).sha, HEAD_SHA); + assert.strictEqual(events[3].event, EventType.Reviewed); + }); + + it('moves only post-review commits when both pre- and post-review commits exist', () => { + const reviewTime = new Date('2024-01-01T12:00:00Z'); + const preReviewSha = 'shaB'; + const events: TimelineEvent[] = [ + makeCommit(LATEST_REVIEW_SHA, new Date('2024-01-01T10:00:00Z')), + makeCommit(preReviewSha, new Date('2024-01-01T11:59:00Z')), + makeReview(reviewTime.toISOString()), + makeCommit(HEAD_SHA, new Date('2024-01-01T13:00:00Z')), + ]; + + insertNewCommitsSinceReview(events, LATEST_REVIEW_SHA, CURRENT_USER, makeHead(HEAD_SHA)); + + assert.strictEqual(events.length, 5); + assert.strictEqual((events[0] as CommitEvent).sha, LATEST_REVIEW_SHA); + assert.strictEqual((events[1] as CommitEvent).sha, preReviewSha); + assert.strictEqual(events[2].event, EventType.Reviewed); + assert.strictEqual(events[3].event, EventType.NewCommitsSinceReview); + assert.strictEqual((events[4] as CommitEvent).sha, HEAD_SHA); + }); + }); }); \ No newline at end of file diff --git a/webviews/activityBarView/index.css b/webviews/activityBarView/index.css index 3fe6623e60..9cb20a1725 100644 --- a/webviews/activityBarView/index.css +++ b/webviews/activityBarView/index.css @@ -240,6 +240,14 @@ button .icon svg { display: flex; flex-grow: 1; min-width: 0; + position: relative; + align-items: center; +} + +.attestation-checkbox-overlay { + position: absolute; + right: calc(50% + 130px + 8px); + margin-right: 0; } .dropdown-container { diff --git a/webviews/common/common.css b/webviews/common/common.css index 9408ac122b..945a0946bb 100644 --- a/webviews/common/common.css +++ b/webviews/common/common.css @@ -209,7 +209,7 @@ input[type='checkbox']:not(:checked):hover { outline: 1px solid var(--vscode-contrastActiveBorder); } -:not(.copilot-icon) > svg path, +:not(.copilot-icon)>svg path, .copilot-icon svg path:first-of-type { fill: var(--vscode-foreground); } @@ -341,6 +341,7 @@ body img.avatar { flex: 1; min-width: 0; } + .reviewer-icons { display: flex; gap: 4px; @@ -407,6 +408,34 @@ body img.avatar { min-width: 80px; } +.attestation-checkbox-wrapper { + display: flex; + align-items: center; + margin-right: 8px; +} + +.attestation-checkbox-label { + margin-left: 4px; + white-space: nowrap; + cursor: pointer; +} + +.comment-actions-right { + display: flex; + flex-grow: 1; + align-items: center; + justify-content: flex-end; + gap: 8px; + min-width: 0; +} + +/* When the dropdown is grouped with the attestation checkbox we don't want it +to stretch and push the checkbox away from the button. */ +.comment-actions-right .dropdown-container.spreadable { + flex-grow: 0; + width: auto; +} + .merge-queue-title .merge-queue-pending { color: var(--vscode-list-warningForeground); } @@ -523,6 +552,7 @@ button.split-right { border: 1px solid var(--vscode-button-border, transparent); border-left: none; } + button.split-right:disabled { cursor: default; } diff --git a/webviews/common/context.tsx b/webviews/common/context.tsx index 3e6a20f4b8..929a8b0cb1 100644 --- a/webviews/common/context.tsx +++ b/webviews/common/context.tsx @@ -11,7 +11,7 @@ import { CloseResult, DescriptionResult, OpenCommitChangesArgs, OpenLocalFileArg import { IComment } from '../../src/common/comment'; import { EventType, ReviewEvent, SessionLinkInfo, TimelineEvent } from '../../src/common/timelineEvent'; import { IProjectItem, MergeMethod, PullRequestCheckStatus, ReadyForReview } from '../../src/github/interface'; -import { CancelCodingAgentReply, ChangeAssigneesReply, ChangeBaseReply, ConvertToDraftReply, DeleteReviewResult, FileUploadCompletedMessage, MergeArguments, MergeResult, ProjectItemsReply, PullRequest, ReadyForReviewReply, SubmitReviewReply, UploadFilesReply } from '../../src/github/views'; +import { CancelCodingAgentReply, ChangeAssigneesReply, ChangeBaseReply, ConvertToDraftReply, DeleteReviewResult, FileUploadCompletedMessage, MergeArguments, MergeResult, ProjectItemsReply, PullRequest, ReadyForReviewReply, SubmitReviewArgs, SubmitReviewReply, UploadFilesReply } from '../../src/github/views'; /** * Encode a {@linkcode Uint8Array} as a base64 string. Uses fixed-size chunks to @@ -179,20 +179,23 @@ export class PRContext { this.updatePR({ pendingCommentDrafts: pendingCommentDrafts }); }; - private async submitReviewCommand(command: string, body: string) { + private async submitReviewCommand(command: string, args: SubmitReviewArgs) { try { - const result: SubmitReviewReply = await this.postMessage({ command, args: body }); + const result: SubmitReviewReply = await this.postMessage({ command, args }); return this.appendReview(result); } catch (error) { return this.updatePR({ busy: false }); } } - public requestChanges = (body: string) => this.submitReviewCommand('pr.request-changes', body); + public requestChanges = (body: string) => + this.submitReviewCommand('pr.request-changes', { body }); - public approve = (body: string) => this.submitReviewCommand('pr.approve', body); + public approve = (body: string, addAttestation: boolean = false) => + this.submitReviewCommand('pr.approve', { body, addAttestation }); - public submit = (body: string) => this.submitReviewCommand('pr.submit', body); + public submit = (body: string) => + this.submitReviewCommand('pr.submit', { body }); private _uploadCompletionHandlers: Map void> = new Map(); diff --git a/webviews/components/comment.tsx b/webviews/components/comment.tsx index 85e9c8cf48..d0d7cf007c 100644 --- a/webviews/components/comment.tsx +++ b/webviews/components/comment.tsx @@ -496,10 +496,13 @@ export function AddComment({ hasReviewDraft, owner, repo, - number + number, + attestationCommitsEnabled, + isCurrentlyCheckedOut }: PullRequest) { const { updatePR, requestChanges, approve, close, openOnGitHub, submit, uploadFilesIntoPendingComment, uploadPastedFilesIntoPendingComment } = useContext(PullRequestContext); const [isBusy, setBusy] = useState(false); + const [addAttestation, setAddAttestation] = useState(false); const form = useRef(); const textareaRef = useRef(); @@ -530,7 +533,7 @@ export function AddComment({ await requestChanges(value); break; case ReviewType.Approve: - await approve(value); + await approve(value, addAttestation); break; default: await submit(value); @@ -538,14 +541,11 @@ export function AddComment({ setBusy(false); } - const onKeyDown = useCallback( - e => { - if ((e.metaKey || e.ctrlKey) && e.key === 'Enter') { - submitAction(currentSelection); - } - }, - [submit], - ); + const onKeyDown = e => { + if ((e.metaKey || e.ctrlKey) && e.key === 'Enter') { + submitAction(currentSelection); + } + }; async function defaultSubmitAction(): Promise { await submitAction(currentSelection); @@ -566,6 +566,8 @@ export function AddComment({ const shouldDisableNonApproveButtons = !pendingCommentText?.trim() && !hasReviewDraft; const shouldDisableApproveButton = false; // Approve is always allowed (when not busy) + const showAttestationCheckbox = !!attestationCommitsEnabled && !!isCurrentlyCheckedOut && !!availableActions.approve && availableActions.approve === COMMENT_METHODS.approve; + return (
} className="comment-form main-comment-form" >
@@ -609,30 +611,46 @@ export function AddComment({ ) : null} - makeCommentMenuContext(owner, repo, number, availableActions, pendingCommentText, shouldDisableNonApproveButtons)} - defaultAction={defaultSubmitAction} - defaultOptionLabel={() => availableActions[currentSelection]!} - defaultOptionValue={() => currentSelection} - allOptions={() => { - const actions: { label: string; value: string; optionDisabled: boolean; action: (event: React.MouseEvent) => void }[] = []; - if (availableActions.comment) { - actions.push({ label: availableActions[ReviewType.Comment]!, value: ReviewType.Comment, action: () => submitAction(ReviewType.Comment), optionDisabled: shouldDisableNonApproveButtons }); - } - if (availableActions.approve) { - actions.push({ label: availableActions[ReviewType.Approve]!, value: ReviewType.Approve, action: () => submitAction(ReviewType.Approve), optionDisabled: shouldDisableApproveButton }); - } - if (availableActions.requestChanges) { - actions.push({ label: availableActions[ReviewType.RequestChanges]!, value: ReviewType.RequestChanges, action: () => submitAction(ReviewType.RequestChanges), optionDisabled: shouldDisableNonApproveButtons }); - } - return actions; - }} - optionsTitle='Submit pull request review' - disabled={isBusy || busy} - hasSingleAction={Object.keys(availableActions).length === 1} - spreadable={true} - primaryOptionValue={ReviewType.Comment} - /> +
+ {showAttestationCheckbox ? ( +
+ setAddAttestation(!addAttestation)} + /> + +
+ ) : null} + + makeCommentMenuContext(owner, repo, number, availableActions, pendingCommentText, shouldDisableNonApproveButtons)} + defaultAction={defaultSubmitAction} + defaultOptionLabel={() => availableActions[currentSelection]!} + defaultOptionValue={() => currentSelection} + allOptions={() => { + const actions: { label: string; value: string; optionDisabled: boolean; action: (event: React.MouseEvent) => void }[] = []; + if (availableActions.comment) { + actions.push({ label: availableActions[ReviewType.Comment]!, value: ReviewType.Comment, action: () => submitAction(ReviewType.Comment), optionDisabled: shouldDisableNonApproveButtons }); + } + if (availableActions.approve) { + actions.push({ label: availableActions[ReviewType.Approve]!, value: ReviewType.Approve, action: () => submitAction(ReviewType.Approve), optionDisabled: shouldDisableApproveButton }); + } + if (availableActions.requestChanges) { + actions.push({ label: availableActions[ReviewType.RequestChanges]!, value: ReviewType.RequestChanges, action: () => submitAction(ReviewType.RequestChanges), optionDisabled: shouldDisableNonApproveButtons }); + } + return actions; + }} + optionsTitle='Submit pull request review' + disabled={isBusy || busy} + hasSingleAction={Object.keys(availableActions).length === 1} + spreadable={true} + primaryOptionValue={ReviewType.Comment} + /> +
); @@ -692,6 +710,7 @@ const makeCommentMenuContext = (owner: string, repo: string, number: number, ava export const AddCommentSimple = (pr: PullRequest) => { const { updatePR, requestChanges, approve, submit, openOnGitHub, uploadFilesIntoPendingComment, uploadPastedFilesIntoPendingComment } = useContext(PullRequestContext); const [isBusy, setBusy] = useState(false); + const [addAttestation, setAddAttestation] = useState(false); const textareaRef = useRef(); let currentSelection: ReviewType = pr.lastReviewType ?? (pr.currentUserReviewState === 'APPROVED' ? ReviewType.Approve : (pr.currentUserReviewState === 'CHANGES_REQUESTED' ? ReviewType.RequestChanges : ReviewType.Comment)); @@ -707,7 +726,7 @@ export const AddCommentSimple = (pr: PullRequest) => { await requestChanges(value); break; case ReviewType.Approve: - await approve(value); + await approve(value, addAttestation); break; default: await submit(value); @@ -723,16 +742,12 @@ export const AddCommentSimple = (pr: PullRequest) => { updatePR({ pendingCommentText: e.target.value }); }; - const onKeyDown = useCallback( - e => { - if ((e.metaKey || e.ctrlKey) && e.key === 'Enter') { - - e.preventDefault(); - defaultSubmitAction(); - } - }, - [submitAction], - ); + const onKeyDown = e => { + if ((e.metaKey || e.ctrlKey) && e.key === 'Enter') { + e.preventDefault(); + defaultSubmitAction(); + } + }; const availableActions: { comment?: string, approve?: string, requestChanges?: string } = pr.isAuthor ? { comment: 'Comment' } @@ -749,6 +764,8 @@ export const AddCommentSimple = (pr: PullRequest) => { const shouldDisableNonApproveButtons = !pr.pendingCommentText?.trim() && !pr.hasReviewDraft; const shouldDisableApproveButton = false; // Approve is always allowed (when not busy) + const showAttestationCheckbox = !!pr.attestationCommitsEnabled && !!pr.isCurrentlyCheckedOut && !!availableActions.approve && availableActions.approve === COMMENT_METHODS.approve; + return (
@@ -774,6 +791,19 @@ export const AddCommentSimple = (pr: PullRequest) => {
+ {showAttestationCheckbox ? ( +
+ setAddAttestation(!addAttestation)} + /> + +
+ ) : null} makeCommentMenuContext(pr.owner, pr.repo, pr.number, availableActions, pr.pendingCommentText, shouldDisableNonApproveButtons)} defaultAction={defaultSubmitAction}