feat: Make plan approval options contextual#1794
feat: Make plan approval options contextual#1794charlesvien wants to merge 2 commits into04-21-add_fullscreen_expand_to_plan_previewfrom
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
6cdea85 to
d0a4b67
Compare
b1c1487 to
70691ec
Compare
d0a4b67 to
d4fec55
Compare
70691ec to
813b1f4
Compare
d4fec55 to
f6b3492
Compare
813b1f4 to
a4d0590
Compare
Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/agent/src/adapters/claude/permissions/permission-handlers.ts
Line: 213
Comment:
**`prePlanMode` can be set to `"plan"` causing silent approval failure**
If `EnterPlanMode` is called while the session is already in plan mode, `prePlanMode` gets set to `"plan"`. `EnterPlanMode` is not in `BASE_ALLOWED_TOOLS` so `isToolAllowedForMode` returns `false` for plan mode, and `handleEnterPlanModeTool` is invoked. When the user later exits plan mode, `buildExitPlanModePermissionOptions("plan")` produces a first option with `optionId: "plan"`. Since `"plan"` is not in the `applyPlanApproval` allow-list (`"auto" | "default" | "acceptEdits" | "bypassPermissions"`), selecting "Yes, continue with plan" falls through to the rejection path — the user's approval is silently treated as a rejection.
Guard against overwriting `prePlanMode` when already in plan mode:
```suggestion
if (session.permissionMode !== "plan") {
session.prePlanMode = session.permissionMode;
}
session.permissionMode = "plan";
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/agent/src/adapters/claude/permissions/permission-options.ts
Line: 95-100
Comment:
**`MODE_LABELS` missing `"plan"` entry**
`CodeExecutionMode` includes `"plan"`, so if `prePlanMode` is ever `"plan"` the label falls back to the raw string `"plan"`. Adding an entry makes the display name explicit and removes reliance on the fallback:
```suggestion
const MODE_LABELS: Record<CodeExecutionMode, string> = {
default: "manual approval",
acceptEdits: "auto-accept edits",
bypassPermissions: "bypass permissions",
auto: "auto mode",
plan: "plan mode",
};
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Make plan approval options contextual" | Re-trigger Greptile |
| ): Promise<ToolPermissionResult> { | ||
| const { session, toolInput } = context; | ||
|
|
||
| session.prePlanMode = session.permissionMode; |
There was a problem hiding this comment.
prePlanMode can be set to "plan" causing silent approval failure
If EnterPlanMode is called while the session is already in plan mode, prePlanMode gets set to "plan". EnterPlanMode is not in BASE_ALLOWED_TOOLS so isToolAllowedForMode returns false for plan mode, and handleEnterPlanModeTool is invoked. When the user later exits plan mode, buildExitPlanModePermissionOptions("plan") produces a first option with optionId: "plan". Since "plan" is not in the applyPlanApproval allow-list ("auto" | "default" | "acceptEdits" | "bypassPermissions"), selecting "Yes, continue with plan" falls through to the rejection path — the user's approval is silently treated as a rejection.
Guard against overwriting prePlanMode when already in plan mode:
| session.prePlanMode = session.permissionMode; | |
| if (session.permissionMode !== "plan") { | |
| session.prePlanMode = session.permissionMode; | |
| } | |
| session.permissionMode = "plan"; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/agent/src/adapters/claude/permissions/permission-handlers.ts
Line: 213
Comment:
**`prePlanMode` can be set to `"plan"` causing silent approval failure**
If `EnterPlanMode` is called while the session is already in plan mode, `prePlanMode` gets set to `"plan"`. `EnterPlanMode` is not in `BASE_ALLOWED_TOOLS` so `isToolAllowedForMode` returns `false` for plan mode, and `handleEnterPlanModeTool` is invoked. When the user later exits plan mode, `buildExitPlanModePermissionOptions("plan")` produces a first option with `optionId: "plan"`. Since `"plan"` is not in the `applyPlanApproval` allow-list (`"auto" | "default" | "acceptEdits" | "bypassPermissions"`), selecting "Yes, continue with plan" falls through to the rejection path — the user's approval is silently treated as a rejection.
Guard against overwriting `prePlanMode` when already in plan mode:
```suggestion
if (session.permissionMode !== "plan") {
session.prePlanMode = session.permissionMode;
}
session.permissionMode = "plan";
```
How can I resolve this? If you propose a fix, please make it concise.| const MODE_LABELS: Record<string, string> = { | ||
| default: "manual approval", | ||
| acceptEdits: "auto-accept edits", | ||
| bypassPermissions: "bypass permissions", | ||
| auto: "auto mode", | ||
| }; |
There was a problem hiding this comment.
MODE_LABELS missing "plan" entry
CodeExecutionMode includes "plan", so if prePlanMode is ever "plan" the label falls back to the raw string "plan". Adding an entry makes the display name explicit and removes reliance on the fallback:
| const MODE_LABELS: Record<string, string> = { | |
| default: "manual approval", | |
| acceptEdits: "auto-accept edits", | |
| bypassPermissions: "bypass permissions", | |
| auto: "auto mode", | |
| }; | |
| const MODE_LABELS: Record<CodeExecutionMode, string> = { | |
| default: "manual approval", | |
| acceptEdits: "auto-accept edits", | |
| bypassPermissions: "bypass permissions", | |
| auto: "auto mode", | |
| plan: "plan mode", | |
| }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/agent/src/adapters/claude/permissions/permission-options.ts
Line: 95-100
Comment:
**`MODE_LABELS` missing `"plan"` entry**
`CodeExecutionMode` includes `"plan"`, so if `prePlanMode` is ever `"plan"` the label falls back to the raw string `"plan"`. Adding an entry makes the display name explicit and removes reliance on the fallback:
```suggestion
const MODE_LABELS: Record<CodeExecutionMode, string> = {
default: "manual approval",
acceptEdits: "auto-accept edits",
bypassPermissions: "bypass permissions",
auto: "auto mode",
plan: "plan mode",
};
```
How can I resolve this? If you propose a fix, please make it concise.
Problem
Plan approval shows N mode options every time, regardless of the user's current mode. This PR makes it contextual to their pre-plan mode similar to claude code.
The issue:
Changes
How did you test this?
Manually