fix: persist ProceedAlways permission outcome in compact mode#3069
Conversation
Compact mode confirmation dialog uses ProceedAlways for "Allow always" option, but persistPermissionOutcome() only handled ProceedAlwaysProject and ProceedAlwaysUser, causing the permission to never be saved. Now ProceedAlways is treated as project scope (same as ProceedAlwaysProject).
📋 Review SummaryThis PR fixes a bug where selecting "Allow always" ( 🔍 General Feedback
🎯 Specific Feedback🟡 High
const scope =
outcome === ToolConfirmationOutcome.ProceedAlwaysUser
? 'user'
: 'project'; // ProceedAlways or ProceedAlwaysProjectAdding a comment clarifying this intent would help future maintainers. 🟢 Medium
🔵 Low
✅ Highlights
|
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
tanzhenxin
left a comment
There was a problem hiding this comment.
Review
Clean, minimal bugfix. persistPermissionOutcome() was silently dropping ProceedAlways (compact mode's "Allow always") because it only handled ProceedAlwaysProject and ProceedAlwaysUser. The fix correctly adds ProceedAlways to the guard and defaults it to project scope.
Verdict
APPROVE — Correct logic, minimal blast radius. A regression test for the new code path would be a nice follow-up.
…#3069) Compact mode confirmation dialog uses ProceedAlways for "Allow always" option, but persistPermissionOutcome() only handled ProceedAlwaysProject and ProceedAlwaysUser, causing the permission to never be saved. Now ProceedAlways is treated as project scope (same as ProceedAlwaysProject).
…#3069) Compact mode confirmation dialog uses ProceedAlways for "Allow always" option, but persistPermissionOutcome() only handled ProceedAlwaysProject and ProceedAlwaysUser, causing the permission to never be saved. Now ProceedAlways is treated as project scope (same as ProceedAlwaysProject).
TLDR
Fixes a bug where selecting "Allow always" in compact mode did not persist the permission.
Key change:
persistPermissionOutcome()now correctly handlesProceedAlwaysoutcome (compact mode's "Allow always" option), treating it as project scope for persistence.Screenshots / Video Demo
N/A — no user-facing visual change. Behavior change can be verified through testing.
Dive Deeper
Background
Sub-agent permission confirmation dialogs use compact mode UI with only three options:
ProceedOnceProceedAlwaysCancelHowever,
persistPermissionOutcome()only handledProceedAlwaysProjectandProceedAlwaysUser, completely ignoringProceedAlways. This caused users selecting "Allow always" to have permissions neither written tosettings.jsonnor updated in the in-memoryPermissionManager, requiring confirmation again on subsequent tool calls.Implementation
Modified
packages/core/src/core/permission-helpers.ts:ProceedAlwaysto the persistence condition check (no longer dropped by early return)ProceedAlwaysas project scope (same behavior asProceedAlwaysProject)Backward compatibility: No impact.
ProceedAlwayswas previously ignored; now it's handled as project scope, matching user expectations.Reviewer Test Plan
.qwen/settings.jsonto verify the corresponding permission rule was writtenTesting Matrix
Local TypeScript compilation passed. Full test matrix not run.
Linked issues / bugs
Resolves #3067