Skip to content

Commit 395197a

Browse files
committed
tools: re-apply confirm response based on promise
This re-applies #263894 and fixes #264023 by removing a surprising behavior in the DeferredPromise where calling complete() with additional values will update the settled `deferred.value` without affecting the original promise (which can of course only resolve once.)
1 parent 151a19f commit 395197a

File tree

3 files changed

+24
-7
lines changed

3 files changed

+24
-7
lines changed

src/vs/base/common/async.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1745,6 +1745,10 @@ export class DeferredPromise<T> {
17451745
}
17461746

17471747
public complete(value: T) {
1748+
if (this.isSettled) {
1749+
return Promise.resolve();
1750+
}
1751+
17481752
return new Promise<void>(resolve => {
17491753
this.completeCallback(value);
17501754
this.outcome = { outcome: DeferredOutcome.Resolved, value };
@@ -1753,6 +1757,10 @@ export class DeferredPromise<T> {
17531757
}
17541758

17551759
public error(err: unknown) {
1760+
if (this.isSettled) {
1761+
return Promise.resolve();
1762+
}
1763+
17561764
return new Promise<void>(resolve => {
17571765
this.errorCallback(err);
17581766
this.outcome = { outcome: DeferredOutcome.Rejected, value: err };

src/vs/base/test/common/async.test.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1210,6 +1210,18 @@ suite('Async', () => {
12101210
assert.strictEqual((await deferred.p.catch(e => e)).name, 'Canceled');
12111211
assert.strictEqual(deferred.isRejected, true);
12121212
});
1213+
1214+
test('retains the original settled value', async () => {
1215+
const deferred = new async.DeferredPromise<number>();
1216+
assert.strictEqual(deferred.isResolved, false);
1217+
deferred.complete(42);
1218+
assert.strictEqual(await deferred.p, 42);
1219+
assert.strictEqual(deferred.isResolved, true);
1220+
1221+
deferred.complete(-1);
1222+
assert.strictEqual(await deferred.p, 42);
1223+
assert.strictEqual(deferred.isResolved, true);
1224+
});
12131225
});
12141226

12151227
suite('Promises.settled', () => {

src/vs/workbench/contrib/chat/common/chatProgressTypes/chatToolInvocation.ts

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -29,9 +29,8 @@ export class ChatToolInvocation implements IChatToolInvocation {
2929
return this._confirmDeferred;
3030
}
3131

32-
private _isConfirmed: ConfirmedReason | undefined;
3332
public get isConfirmed(): ConfirmedReason | undefined {
34-
return this._isConfirmed;
33+
return this._confirmDeferred.value;
3534
}
3635

3736
private _resultDetails: IToolResult['toolResultDetails'] | undefined;
@@ -65,12 +64,10 @@ export class ChatToolInvocation implements IChatToolInvocation {
6564

6665
if (!this._confirmationMessages) {
6766
// No confirmation needed
68-
this._isConfirmed = { type: ToolConfirmKind.ConfirmationNotNeeded };
69-
this._confirmDeferred.complete(this._isConfirmed);
67+
this._confirmDeferred.complete({ type: ToolConfirmKind.ConfirmationNotNeeded });
7068
}
7169

72-
this._confirmDeferred.p.then(confirmed => {
73-
this._isConfirmed = confirmed;
70+
this._confirmDeferred.p.then(() => {
7471
this._confirmationMessages = undefined;
7572
});
7673

@@ -107,7 +104,7 @@ export class ChatToolInvocation implements IChatToolInvocation {
107104
invocationMessage: this.invocationMessage,
108105
pastTenseMessage: this.pastTenseMessage,
109106
originMessage: this.originMessage,
110-
isConfirmed: this._isConfirmed,
107+
isConfirmed: this._confirmDeferred.value,
111108
isComplete: true,
112109
source: this.source,
113110
resultDetails: isToolResultOutputDetails(this._resultDetails)

0 commit comments

Comments
 (0)