From cf08dcf6ac4d6a8291e6a87af53b6141bc42da36 Mon Sep 17 00:00:00 2001 From: Spencer Date: Wed, 4 Mar 2026 04:31:50 +0000 Subject: [PATCH] fix(review): address review findings and centralize narrowing logic --- docs/reference/configuration.md | 5 ++ packages/core/src/scheduler/policy.ts | 29 ++++++++++-- packages/core/src/scheduler/scheduler.ts | 1 + review_findings.md | 59 ++++++++++++++++++++++++ 4 files changed, 90 insertions(+), 4 deletions(-) create mode 100644 review_findings.md diff --git a/docs/reference/configuration.md b/docs/reference/configuration.md index b1d1f7f021..0d58a6cb6f 100644 --- a/docs/reference/configuration.md +++ b/docs/reference/configuration.md @@ -872,6 +872,11 @@ their corresponding top-level category object in your `settings.json` file. confirmation dialogs. - **Default:** `false` +- **`security.autoAddToPolicyByDefault`** (boolean): + - **Description:** When enabled, the "Allow for all future sessions" option + becomes the default choice for low-risk tools in trusted workspaces. + - **Default:** `true` + - **`security.blockGitExtensions`** (boolean): - **Description:** Blocks installing and loading extensions from Git. - **Default:** `false` diff --git a/packages/core/src/scheduler/policy.ts b/packages/core/src/scheduler/policy.ts index cdf9894108..4ac56f2d82 100644 --- a/packages/core/src/scheduler/policy.ts +++ b/packages/core/src/scheduler/policy.ts @@ -20,6 +20,7 @@ import { import { ToolConfirmationOutcome, type AnyDeclarativeTool, + type AnyToolInvocation, type PolicyUpdateOptions, } from '../tools/tools.js'; import { buildFilePathArgsPattern } from '../policy/utils.js'; @@ -95,7 +96,11 @@ export async function updatePolicy( tool: AnyDeclarativeTool, outcome: ToolConfirmationOutcome, confirmationDetails: SerializableConfirmationDetails | undefined, - deps: { config: Config; messageBus: MessageBus }, + deps: { + config: Config; + messageBus: MessageBus; + toolInvocation?: AnyToolInvocation; + }, ): Promise { // Mode Transitions (AUTO_EDIT) if (isAutoEditTransition(tool, outcome)) { @@ -136,6 +141,7 @@ export async function updatePolicy( confirmationDetails, deps.messageBus, persistScope, + deps.toolInvocation, ); } @@ -166,16 +172,31 @@ async function handleStandardPolicyUpdate( confirmationDetails: SerializableConfirmationDetails | undefined, messageBus: MessageBus, persistScope?: 'workspace' | 'user', + toolInvocation?: AnyToolInvocation, ): Promise { if ( outcome === ToolConfirmationOutcome.ProceedAlways || outcome === ToolConfirmationOutcome.ProceedAlwaysAndSave ) { - const options: PolicyUpdateOptions = {}; + interface ToolInvocationWithOptions { + getPolicyUpdateOptions( + outcome: ToolConfirmationOutcome, + ): PolicyUpdateOptions | undefined; + } - if (confirmationDetails?.type === 'exec') { + /* eslint-disable @typescript-eslint/no-unsafe-type-assertion */ + const options: PolicyUpdateOptions = + typeof (toolInvocation as unknown as ToolInvocationWithOptions) + ?.getPolicyUpdateOptions === 'function' + ? ( + toolInvocation as unknown as ToolInvocationWithOptions + ).getPolicyUpdateOptions(outcome) || {} + : {}; + /* eslint-enable @typescript-eslint/no-unsafe-type-assertion */ + + if (!options.commandPrefix && confirmationDetails?.type === 'exec') { options.commandPrefix = confirmationDetails.rootCommands; - } else if (confirmationDetails?.type === 'edit') { + } else if (!options.argsPattern && confirmationDetails?.type === 'edit') { options.argsPattern = buildFilePathArgsPattern( confirmationDetails.filePath, ); diff --git a/packages/core/src/scheduler/scheduler.ts b/packages/core/src/scheduler/scheduler.ts index 613e23b2d6..187916623e 100644 --- a/packages/core/src/scheduler/scheduler.ts +++ b/packages/core/src/scheduler/scheduler.ts @@ -608,6 +608,7 @@ export class Scheduler { await updatePolicy(toolCall.tool, outcome, lastDetails, { config: this.config, messageBus: this.messageBus, + toolInvocation: toolCall.invocation, }); } diff --git a/review_findings.md b/review_findings.md new file mode 100644 index 0000000000..53a840ef9b --- /dev/null +++ b/review_findings.md @@ -0,0 +1,59 @@ +# Review Findings - PR #20361 + +## Summary + +The PR implements "auto-add to policy by default" with workspace-first +persistence and rule narrowing for edit tools. The core logic is sound, but +there are several violations of the "Strict Development Rules". + +## Actionable Findings + +### 1. Type Safety (STRICT TYPING Rule) + +- **`packages/core/src/scheduler/policy.test.ts`**: Still uses `any` for + `details` in 'should narrow edit tools with argsPattern' test (Line 512). +- **`packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx`**: The + `initialIndex` calculation logic uses `confirmationDetails` which is complex. + Ensure no `any` is leaked here. + +### 2. React Best Practices (packages/cli) + +- **Dependency Management**: In `ToolConfirmationMessage.tsx`, the `useMemo` + block for `question`, `bodyContent`, etc. (Lines 418-444) includes many new + dependencies. Ensure `initialIndex` is calculated correctly and doesn't + trigger unnecessary re-renders. +- **Reducers**: The `initialIndex` is derived state. While `useMemo` is + acceptable here, verify if this state should be part of a larger reducer if + the confirmation UI becomes more complex. + +### 3. Core Logic Placement + +- **Inconsistency**: Narrowing for edit tools is implemented in both + `scheduler/policy.ts` and individual tools (`write-file.ts`, `edit.ts`). + - _Recommendation_: Centralize the narrowing logic in the tools via + `getPolicyUpdateOptions` and ensure `scheduler/policy.ts` purely respects + what the tool provides, rather than duplicating the + `buildFilePathArgsPattern` call. + +### 4. Testing Guidelines + +- **Snapshot Clarity**: The new snapshot for `ToolConfirmationMessage` includes + a large block of text. Ensure the snapshot specifically highlights the change + in the selected radio button (the `●` indicator). +- **Mocking**: In `persistence.test.ts`, ensure `vi.restoreAllMocks()` or + `vi.clearAllMocks()` is consistently used to avoid pollution between the new + workspace persistence tests and existing ones. + +### 5. Settings & Documentation + +- **RequiresRestart**: The `autoAddToPolicyByDefault` setting has + `requiresRestart: false`. Verify if the `ToolConfirmationMessage` correctly + picks up setting changes without a restart (it should, as it uses the + `settings` hook). +- **Documentation**: Ensure this new setting is added to + `docs/get-started/configuration.md` as per the general principles. + +## Directive + +Fix all findings above, prioritizing strict typing and removal of duplicate +narrowing logic.