Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/core/src/agents/remote-invocation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,8 @@ export class RemoteAgentInvocation extends BaseToolInvocation<
type: 'info',
title: `Call Remote Agent: ${this.definition.displayName ?? this.definition.name}`,
prompt: `Calling remote agent: "${this.params.query}"`,
onConfirm: async (outcome: ToolConfirmationOutcome) => {
await this.publishPolicyUpdate(outcome);
onConfirm: async (_outcome: ToolConfirmationOutcome) => {
// Policy updates are now handled centrally by the scheduler
},
};
}
Expand Down
37 changes: 15 additions & 22 deletions packages/core/src/tools/confirmation-policy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,17 +136,17 @@ describe('Tool Confirmation Policy Updates', () => {
it.each([
{
outcome: ToolConfirmationOutcome.ProceedAlways,
shouldPublish: false,
_shouldPublish: false,
expectedApprovalMode: ApprovalMode.AUTO_EDIT,
},
{
outcome: ToolConfirmationOutcome.ProceedAlwaysAndSave,
shouldPublish: true,
persist: true,
_shouldPublish: true,
_persist: true,
},
])(
'should handle $outcome correctly',
async ({ outcome, shouldPublish, persist, expectedApprovalMode }) => {
async ({ outcome, expectedApprovalMode }) => {
const tool = create(mockConfig, mockMessageBus);

// For file-based tools, ensure the file exists if needed
Expand All @@ -172,26 +172,19 @@ describe('Tool Confirmation Policy Updates', () => {
if (confirmation) {
await confirmation.onConfirm(outcome);

if (shouldPublish) {
expect(mockMessageBus.publish).toHaveBeenCalledWith(
expect.objectContaining({
type: MessageBusType.UPDATE_POLICY,
persist,
}),
);
} else {
// Should not publish UPDATE_POLICY message for ProceedAlways
const publishCalls = (mockMessageBus.publish as any).mock.calls;
const hasUpdatePolicy = publishCalls.some(
(call: any) => call[0].type === MessageBusType.UPDATE_POLICY,
);
expect(hasUpdatePolicy).toBe(false);
}
// Policy updates are no longer published by onConfirm; they are
// handled centrally by the schedulers.
const publishCalls = (mockMessageBus.publish as any).mock.calls;
const hasUpdatePolicy = publishCalls.some(
(call: any) => call[0].type === MessageBusType.UPDATE_POLICY,
);
expect(hasUpdatePolicy).toBe(false);

if (expectedApprovalMode !== undefined) {
expect(mockConfig.setApprovalMode).toHaveBeenCalledWith(
expectedApprovalMode,
);
// expectedApprovalMode in this test (AUTO_EDIT) is now handled
// by updatePolicy in the scheduler, so it should not be called
// here either.
expect(mockConfig.setApprovalMode).not.toHaveBeenCalled();
}
}
},
Expand Down
13 changes: 4 additions & 9 deletions packages/core/src/tools/edit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
BaseToolInvocation,
Kind,
type ToolCallConfirmationDetails,
ToolConfirmationOutcome,
type ToolConfirmationOutcome,
type ToolEditConfirmationDetails,
type ToolInvocation,
type ToolLocation,
Expand Down Expand Up @@ -725,14 +725,9 @@ class EditToolInvocation
fileDiff,
originalContent: editData.currentContent,
newContent: editData.newContent,
onConfirm: async (outcome: ToolConfirmationOutcome) => {
if (outcome === ToolConfirmationOutcome.ProceedAlways) {
// No need to publish a policy update as the default policy for
// AUTO_EDIT already reflects always approving edit.
this.config.setApprovalMode(ApprovalMode.AUTO_EDIT);
} else {
await this.publishPolicyUpdate(outcome);
}
onConfirm: async (_outcome: ToolConfirmationOutcome) => {
// Mode transitions (e.g. AUTO_EDIT) and policy updates are now
// handled centrally by the scheduler.

if (ideConfirmation) {
const result = await ideConfirmation;
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/tools/enter-plan-mode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ export class EnterPlanModeInvocation extends BaseToolInvocation<
'This will restrict the agent to read-only tools to allow for safe planning.',
onConfirm: async (outcome: ToolConfirmationOutcome) => {
this.confirmationOutcome = outcome;
await this.publishPolicyUpdate(outcome);
// Policy updates are now handled centrally by the scheduler
},
};
}
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/tools/mcp-tool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ export class DiscoveredMCPToolInvocation extends BaseToolInvocation<
DiscoveredMCPToolInvocation.allowlist.add(toolAllowListKey);
} else if (outcome === ToolConfirmationOutcome.ProceedAlwaysAndSave) {
DiscoveredMCPToolInvocation.allowlist.add(toolAllowListKey);
await this.publishPolicyUpdate(outcome);
// Persistent policy updates are now handled centrally by the scheduler
}
},
};
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/tools/memoryTool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ class MemoryToolInvocation extends BaseToolInvocation<
if (outcome === ToolConfirmationOutcome.ProceedAlways) {
MemoryToolInvocation.allowlist.add(allowlistKey);
}
await this.publishPolicyUpdate(outcome);
// Policy updates are now handled centrally by the scheduler
},
};
return confirmationDetails;
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/tools/shell.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,8 @@ export class ShellToolInvocation extends BaseToolInvocation<
command: this.params.command,
rootCommand: rootCommandDisplay,
rootCommands,
onConfirm: async (outcome: ToolConfirmationOutcome) => {
await this.publishPolicyUpdate(outcome);
onConfirm: async (_outcome: ToolConfirmationOutcome) => {
// Policy updates are now handled centrally by the scheduler
},
};
return confirmationDetails;
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/tools/tools.ts
Original file line number Diff line number Diff line change
Expand Up @@ -173,8 +173,8 @@ export abstract class BaseToolInvocation<
type: 'info',
title: `Confirm: ${this._toolDisplayName || this._toolName}`,
prompt: this.getDescription(),
onConfirm: async (outcome: ToolConfirmationOutcome) => {
await this.publishPolicyUpdate(outcome);
onConfirm: async (_outcome: ToolConfirmationOutcome) => {
// Policy updates are now handled centrally by the scheduler
},
};
return confirmationDetails;
Expand Down
7 changes: 3 additions & 4 deletions packages/core/src/tools/web-fetch.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ describe('WebFetchTool', () => {
expect(confirmationDetails).toBe(false);
});

it('should call setApprovalMode when onConfirm is called with ProceedAlways', async () => {
it('should NOT call setApprovalMode when onConfirm is called with ProceedAlways (now handled by scheduler)', async () => {
const tool = new WebFetchTool(mockConfig, bus);
const params = { prompt: 'fetch https://example.com' };
const invocation = tool.build(params);
Expand All @@ -408,9 +408,8 @@ describe('WebFetchTool', () => {
);
}

expect(mockConfig.setApprovalMode).toHaveBeenCalledWith(
ApprovalMode.AUTO_EDIT,
);
// Schedulers are now responsible for mode transitions via updatePolicy
expect(mockConfig.setApprovalMode).not.toHaveBeenCalled();
});
});

Expand Down
13 changes: 4 additions & 9 deletions packages/core/src/tools/web-fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
BaseDeclarativeTool,
BaseToolInvocation,
Kind,
ToolConfirmationOutcome,
type ToolConfirmationOutcome,
} from './tools.js';
import type { MessageBus } from '../confirmation-bus/message-bus.js';
import { ToolErrorType } from './tool-error.js';
Expand Down Expand Up @@ -246,14 +246,9 @@ ${textContent}
title: `Confirm Web Fetch`,
prompt: this.params.prompt,
urls,
onConfirm: async (outcome: ToolConfirmationOutcome) => {
if (outcome === ToolConfirmationOutcome.ProceedAlways) {
// No need to publish a policy update as the default policy for
// AUTO_EDIT already reflects always approving web-fetch.
this.config.setApprovalMode(ApprovalMode.AUTO_EDIT);
} else {
await this.publishPolicyUpdate(outcome);
}
onConfirm: async (_outcome: ToolConfirmationOutcome) => {
// Mode transitions (e.g. AUTO_EDIT) and policy updates are now
// handled centrally by the scheduler.
},
};
return confirmationDetails;
Expand Down
13 changes: 4 additions & 9 deletions packages/core/src/tools/write-file.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import {
BaseDeclarativeTool,
BaseToolInvocation,
Kind,
ToolConfirmationOutcome,
type ToolConfirmationOutcome,
} from './tools.js';
import { ToolErrorType } from './tool-error.js';
import { makeRelative, shortenPath } from '../utils/paths.js';
Expand Down Expand Up @@ -228,14 +228,9 @@ class WriteFileToolInvocation extends BaseToolInvocation<
fileDiff,
originalContent,
newContent: correctedContent,
onConfirm: async (outcome: ToolConfirmationOutcome) => {
if (outcome === ToolConfirmationOutcome.ProceedAlways) {
// No need to publish a policy update as the default policy for
// AUTO_EDIT already reflects always approving write-file.
this.config.setApprovalMode(ApprovalMode.AUTO_EDIT);
} else {
await this.publishPolicyUpdate(outcome);
}
onConfirm: async (_outcome: ToolConfirmationOutcome) => {
// Mode transitions (e.g. AUTO_EDIT) and policy updates are now
// handled centrally by the scheduler.

if (ideConfirmation) {
const result = await ideConfirmation;
Expand Down
Loading