Skip to content

Commit ea32f80

Browse files
benibenjCopilot
benibenj
and
Copilot
authored
agentHost: address agent merge review feedback (#331158)
* agentHost: address agent merge review feedback Follow-ups to PR #331010 review comments: - Rename settings from `chat.agentHost.agentMerge.*` to `chat.agentMerge.*`. - Reject client writes to host-owned `agentMerge.controller` session config so a forged controller state cannot drive a native merge. - Reconcile injected autonomy configuration every cycle and roll back keys that policy later revokes; never widen configuration while a turn is active. - Revalidate the merge target before starting a turn and before merging, and refresh live state, config and top-level comments inside the merge step. - Split `AgentMergeRepairAction` out of `AgentMergeAction` so repair paths cannot express a merge. - Carry all feedback comments per review thread and bound the prompt with explicit caps and an aggregate character budget. - Fail closed when fork head provenance is missing, and ignore refs from a different GitHub host. - Cancel the controller turn when the runtime stops. - Split the enable/disable command preconditions with a dedicated context key and make the configure quick pick's reset a title button. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * agentHost: address agent merge PR review feedback - Preserve host-written session config across a client `SessionConfigChanged` with `replace: true`. Omitting `agentMerge.controller` previously cleared the bound target, comment watermark and attempt budgets, which bypassed the authorization boundary that explicit-write rejection was meant to enforce. - Canonicalize pull request web hosts to their API host when checking that the credential matches. GitHub Enterprise Cloud serves `tenant.ghe.com` from `api.tenant.ghe.com`, so comparing the web host rejected every GHE Cloud pull request. The derivation now reuses `deriveGitHubEndpoints`. - Refresh top-level comments inside `prepareMerge`, last, before the snapshot and token are captured. Refreshing them in the controller beforehand left a window across `prepareMerge`'s own authoritative refreshes in which a new maintainer comment could be missed; refreshing afterwards would invalidate the preparation generation. A comment landing after capture now invalidates the preparation. - Migrate the legacy `chat.agentHost.agentMerge.*` setting ids to their new `chat.agentMerge.*` names so an explicit opt-out is not silently discarded. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 156357c commit ea32f80

11 files changed

Lines changed: 628 additions & 136 deletions

File tree

‎src/vs/platform/agentHost/common/agentHostStarter.config.contribution.ts‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -102,39 +102,39 @@ configurationRegistry.registerConfiguration({
102102
properties: {
103103
[AgentMergeSettingId.Enabled]: {
104104
type: 'boolean',
105-
description: nls.localize('chat.agentHost.agentMerge.enabled', "Enables the experimental Agent Merge controller and its commands. Agent Merge can monitor an agent session's pull request, ask the agent to address selected blockers, and optionally merge the pull request when it is ready."),
105+
description: nls.localize('chat.agentMerge.enabled', "Enables the experimental Agent Merge controller and its commands. Agent Merge can monitor an agent session's pull request, ask the agent to address selected blockers, and optionally merge the pull request when it is ready."),
106106
default: product.quality !== 'stable',
107107
scope: ConfigurationScope.APPLICATION,
108108
tags: ['experimental'],
109109
agentHost: { key: AgentMergeConfigKey.Enabled },
110110
},
111111
[AgentMergeSettingId.AddressReviews]: {
112112
type: 'boolean',
113-
description: nls.localize('chat.agentHost.agentMerge.addressReviews', "Controls whether enabled Agent Merge sessions address unresolved review threads, changes-requested reviews, and new pull request comments from repository maintainers or the Copilot pull request reviewer."),
113+
description: nls.localize('chat.agentMerge.addressReviews', "Controls whether enabled Agent Merge sessions address unresolved review threads, changes-requested reviews, and new pull request comments from repository maintainers or the Copilot pull request reviewer."),
114114
default: true,
115115
scope: ConfigurationScope.APPLICATION,
116116
tags: ['experimental'],
117117
agentHost: { key: AgentMergeConfigKey.AddressReviews },
118118
},
119119
[AgentMergeSettingId.FixCI]: {
120120
type: 'boolean',
121-
description: nls.localize('chat.agentHost.agentMerge.fixCI', "Controls whether enabled Agent Merge sessions ask the agent to fix failed required CI checks."),
121+
description: nls.localize('chat.agentMerge.fixCI', "Controls whether enabled Agent Merge sessions ask the agent to fix failed required CI checks."),
122122
default: true,
123123
scope: ConfigurationScope.APPLICATION,
124124
tags: ['experimental'],
125125
agentHost: { key: AgentMergeConfigKey.FixCI },
126126
},
127127
[AgentMergeSettingId.ResolveConflicts]: {
128128
type: 'boolean',
129-
description: nls.localize('chat.agentHost.agentMerge.resolveConflicts', "Controls whether enabled Agent Merge sessions ask the agent to update branches that are behind or resolve merge conflicts."),
129+
description: nls.localize('chat.agentMerge.resolveConflicts', "Controls whether enabled Agent Merge sessions ask the agent to update branches that are behind or resolve merge conflicts."),
130130
default: true,
131131
scope: ConfigurationScope.APPLICATION,
132132
tags: ['experimental'],
133133
agentHost: { key: AgentMergeConfigKey.ResolveConflicts },
134134
},
135135
[AgentMergeSettingId.MergePullRequest]: {
136136
type: 'boolean',
137-
description: nls.localize('chat.agentHost.agentMerge.mergePullRequest', "Controls whether the Agent Host automatically merges or enqueues pull requests for enabled Agent Merge sessions after all selected maintenance work is complete."),
137+
description: nls.localize('chat.agentMerge.mergePullRequest', "Controls whether the Agent Host automatically merges or enqueues pull requests for enabled Agent Merge sessions after all selected maintenance work is complete."),
138138
default: false,
139139
scope: ConfigurationScope.APPLICATION,
140140
tags: ['experimental'],
@@ -144,20 +144,20 @@ configurationRegistry.registerConfiguration({
144144
type: 'string',
145145
enum: ['auto', 'squash', 'merge', 'rebase'],
146146
enumDescriptions: [
147-
nls.localize('chat.agentHost.agentMerge.mergeMethod.auto', "Uses the first repository-compatible method in this order: squash, merge commit, rebase."),
148-
nls.localize('chat.agentHost.agentMerge.mergeMethod.squash', "Uses squash merge when the repository permits it."),
149-
nls.localize('chat.agentHost.agentMerge.mergeMethod.merge', "Uses a merge commit when the repository permits it."),
150-
nls.localize('chat.agentHost.agentMerge.mergeMethod.rebase', "Uses rebase merge when the repository permits it."),
147+
nls.localize('chat.agentMerge.mergeMethod.auto', "Uses the first repository-compatible method in this order: squash, merge commit, rebase."),
148+
nls.localize('chat.agentMerge.mergeMethod.squash', "Uses squash merge when the repository permits it."),
149+
nls.localize('chat.agentMerge.mergeMethod.merge', "Uses a merge commit when the repository permits it."),
150+
nls.localize('chat.agentMerge.mergeMethod.rebase', "Uses rebase merge when the repository permits it."),
151151
],
152-
description: nls.localize('chat.agentHost.agentMerge.mergeMethod', "Controls the native merge method used by Agent Merge."),
152+
description: nls.localize('chat.agentMerge.mergeMethod', "Controls the native merge method used by Agent Merge."),
153153
default: 'auto',
154154
scope: ConfigurationScope.APPLICATION,
155155
tags: ['experimental'],
156156
agentHost: { key: AgentMergeConfigKey.MergeMethod },
157157
},
158158
[AgentMergeSettingId.ReplyAttribution]: {
159159
type: 'boolean',
160-
description: nls.localize('chat.agentHost.agentMerge.replyAttribution', "Controls whether review-thread replies posted by Agent Merge include an automated-reply attribution note."),
160+
description: nls.localize('chat.agentMerge.replyAttribution', "Controls whether review-thread replies posted by Agent Merge include an automated-reply attribution note."),
161161
default: true,
162162
scope: ConfigurationScope.APPLICATION,
163163
tags: ['experimental'],

‎src/vs/platform/agentHost/common/agentMerge.ts‎

Lines changed: 72 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -19,16 +19,23 @@ export const AgentMergeConfigKey = {
1919
} as const;
2020

2121
export const AgentMergeSettingId = {
22-
Enabled: 'chat.agentHost.agentMerge.enabled',
23-
AddressReviews: 'chat.agentHost.agentMerge.addressReviews',
24-
FixCI: 'chat.agentHost.agentMerge.fixCI',
25-
ResolveConflicts: 'chat.agentHost.agentMerge.resolveConflicts',
26-
MergePullRequest: 'chat.agentHost.agentMerge.mergePullRequest',
27-
MergeMethod: 'chat.agentHost.agentMerge.mergeMethod',
28-
ReplyAttribution: 'chat.agentHost.agentMerge.replyAttribution',
22+
Enabled: 'chat.agentMerge.enabled',
23+
AddressReviews: 'chat.agentMerge.addressReviews',
24+
FixCI: 'chat.agentMerge.fixCI',
25+
ResolveConflicts: 'chat.agentMerge.resolveConflicts',
26+
MergePullRequest: 'chat.agentMerge.mergePullRequest',
27+
MergeMethod: 'chat.agentMerge.mergeMethod',
28+
ReplyAttribution: 'chat.agentMerge.replyAttribution',
2929
} as const;
3030

31-
export type AgentMergeAction = 'addressReviews' | 'fixCI' | 'resolveConflicts' | 'mergePullRequest';
31+
/**
32+
* Work the agent itself can be asked to perform. Merging is deliberately absent:
33+
* it is executed by the host, never delegated to a model.
34+
*/
35+
export type AgentMergeRepairAction = 'addressReviews' | 'fixCI' | 'resolveConflicts';
36+
37+
/** A user-authorizable Agent Merge action, including the host-executed merge. */
38+
export type AgentMergeAction = AgentMergeRepairAction | 'mergePullRequest';
3239
export type AgentMergeMethod = 'auto' | 'squash' | 'merge' | 'rebase';
3340

3441
export interface AgentMergeActions {
@@ -61,6 +68,11 @@ export interface AgentMergeReviewThreadContext {
6168
readonly id: string;
6269
readonly path?: string;
6370
readonly line?: number;
71+
/** Authorized comments in the thread, oldest first, so later follow-ups are visible. */
72+
readonly comments: readonly AgentMergeFeedbackComment[];
73+
}
74+
75+
export interface AgentMergeFeedbackComment {
6476
readonly author?: string;
6577
readonly body: string;
6678
}
@@ -146,7 +158,7 @@ export type AgentMergeGateResult =
146158
| { readonly kind: 'indeterminate'; readonly reason: string }
147159
| { readonly kind: 'terminal' }
148160
| { readonly kind: 'noWork'; readonly waitingOnChecks: boolean; readonly fingerprint: string }
149-
| { readonly kind: 'prompt'; readonly actions: readonly AgentMergeAction[]; readonly fingerprint: string; readonly context: AgentMergePromptContext }
161+
| { readonly kind: 'prompt'; readonly actions: readonly AgentMergeRepairAction[]; readonly fingerprint: string; readonly context: AgentMergePromptContext }
150162
| { readonly kind: 'merge'; readonly fingerprint: string };
151163

152164
export interface AgentMergePromptContext {
@@ -156,14 +168,23 @@ export interface AgentMergePromptContext {
156168
readonly baseRef: string;
157169
readonly headRef: string;
158170
readonly reviewThreads: readonly AgentMergeReviewThreadContext[];
159-
readonly reviewSummaries: readonly string[];
160-
readonly newComments: readonly string[];
171+
readonly reviewSummaries: readonly AgentMergeFeedbackComment[];
172+
readonly newComments: readonly AgentMergeFeedbackComment[];
161173
readonly failedChecks: readonly string[];
162174
readonly behind: boolean;
163175
readonly conflicting: boolean;
164176
readonly commentWatermark: string;
165177
}
166178

179+
/** Caps that keep an autonomous prompt bounded regardless of pull request size. */
180+
const maximumReviewThreads = 10;
181+
const maximumCommentsPerThread = 5;
182+
const maximumReviewSummaries = 5;
183+
const maximumNewComments = 5;
184+
const maximumFailedChecks = 20;
185+
const maximumFeedbackBodyLength = 1_000;
186+
const maximumFeedbackBudget = 20_000;
187+
167188
const maintainerAssociations = new Set(['OWNER', 'MEMBER', 'COLLABORATOR']);
168189
const copilotPullRequestReviewerId = '175728472';
169190
const copilotPullRequestReviewerLogins = new Set(['copilot', 'copilot-pull-request-reviewer[bot]']);
@@ -247,6 +268,28 @@ function isRecord(value: unknown): value is Record<string, unknown> {
247268
return typeof value === 'object' && value !== null && !Array.isArray(value);
248269
}
249270

271+
/**
272+
* Truncates feedback bodies against one shared character budget so a
273+
* comment-heavy pull request cannot grow the autonomous prompt without bound.
274+
*/
275+
class FeedbackBudget {
276+
277+
private _remaining: number;
278+
279+
constructor(budget: number) {
280+
this._remaining = budget;
281+
}
282+
283+
take(author: string | undefined, body: string | undefined): AgentMergeFeedbackComment {
284+
const text = (body ?? '').slice(0, Math.max(0, Math.min(maximumFeedbackBodyLength, this._remaining)));
285+
this._remaining -= text.length;
286+
return {
287+
...(author ? { author } : {}),
288+
body: text,
289+
};
290+
}
291+
}
292+
250293
export function evaluateAgentMerge(snapshot: PullRequestSnapshot, configuration: AgentMergeConfiguration, commentWatermark: string): AgentMergeGateResult {
251294
const core = snapshot.core;
252295
if (core.status !== 'ready' || !core.complete || !core.value) {
@@ -281,7 +324,7 @@ export function evaluateAgentMerge(snapshot: PullRequestSnapshot, configuration:
281324
const mergeability = snapshot.mergeability.value!;
282325
const behind = mergeability.mergeStateStatus?.toUpperCase() === 'BEHIND';
283326
const conflicting = mergeability.mergeable === 'CONFLICTING';
284-
const actions: AgentMergeAction[] = [];
327+
const actions: AgentMergeRepairAction[] = [];
285328
if (configuration.addressReviews && (reviewThreads.length > 0 || changesRequested.length > 0 || newComments.length > 0)) {
286329
actions.push('addressReviews');
287330
}
@@ -292,25 +335,29 @@ export function evaluateAgentMerge(snapshot: PullRequestSnapshot, configuration:
292335
actions.push('resolveConflicts');
293336
}
294337

338+
const budget = new FeedbackBudget(maximumFeedbackBudget);
295339
const context: AgentMergePromptContext = {
296340
pullRequestUrl: core.value.url,
297341
title: core.value.title,
298342
headSha: core.value.headSha,
299343
baseRef: core.value.baseRef,
300344
headRef: core.value.headRef,
301-
reviewThreads: reviewThreads.slice(0, 20).map(thread => {
302-
const comment = thread.comments.find(candidate => isAgentMergeFeedbackAuthor(candidate.author));
303-
return {
304-
id: thread.id,
305-
...(thread.path ? { path: thread.path } : {}),
306-
...(thread.line !== undefined ? { line: thread.line } : {}),
307-
...(comment?.author?.login ? { author: comment.author.login } : {}),
308-
body: (comment?.body ?? '').slice(0, 1_000),
309-
};
310-
}),
311-
reviewSummaries: changesRequested.map(review => review.body ?? `Changes requested by ${review.author?.login ?? 'reviewer'}`),
312-
newComments: newComments.map(comment => comment.body ?? `Comment by ${comment.author?.login ?? 'reviewer'}`),
313-
failedChecks: checks.failed.map(check => check.name),
345+
reviewThreads: reviewThreads.slice(0, maximumReviewThreads).map(thread => ({
346+
id: thread.id,
347+
...(thread.path ? { path: thread.path } : {}),
348+
...(thread.line !== undefined ? { line: thread.line } : {}),
349+
comments: thread.comments
350+
.filter(comment => isAgentMergeFeedbackAuthor(comment.author))
351+
.slice(-maximumCommentsPerThread)
352+
.map(comment => budget.take(comment.author?.login, comment.body)),
353+
})),
354+
reviewSummaries: changesRequested
355+
.slice(-maximumReviewSummaries)
356+
.map(review => budget.take(review.author?.login, review.body)),
357+
newComments: newComments
358+
.slice(-maximumNewComments)
359+
.map(comment => budget.take(comment.author?.login, comment.body)),
360+
failedChecks: checks.failed.slice(0, maximumFailedChecks).map(check => check.name),
314361
behind,
315362
conflicting,
316363
commentWatermark: newComments.reduce((latest, comment) => comment.createdAt && comment.createdAt > latest ? comment.createdAt : latest, commentWatermark),

0 commit comments

Comments
 (0)