-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(core): require opt-in for plan mode prompt #5433
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -18,16 +18,16 @@ const debugLogger = createDebugLogger('ENTER_PLAN_MODE'); | |||||
|
|
||||||
| export type EnterPlanModeParams = Record<string, never>; | ||||||
|
|
||||||
| const enterPlanModeToolDescription = `Use this tool to lower into plan mode before doing uncertain or complex work. Entering plan mode is a privilege reduction, so it does not require user confirmation. | ||||||
| const enterPlanModeToolDescription = `Use this tool only after the user explicitly asks to switch into plan mode or confirms they want plan mode. Entering plan mode is a privilege reduction, so it does not require user confirmation at execution time. | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The tool description lists two opt-in conditions ("explicitly asks", "confirms they want plan mode") but omits the third from the system prompt: "has already enabled it." A model reading the tool description alone wouldn't know plan mode is permissible when already active via
Suggested change
— qwen3.7-max via Qwen Code /review |
||||||
|
|
||||||
| ## When to Use This Tool | ||||||
| Use this tool when the task is not yet clear enough to safely execute, for example when it requires multi-file changes, design choices, investigation before a plan can be summarized, or when requirements are ambiguous. While investigating, if complexity rises or you find yourself repeatedly needing to ask the user, enter plan mode and consolidate a plan. | ||||||
| Use this tool when the user has opted into plan mode for a task that should be read-only while the plan is formed, such as multi-file changes, design choices, or ambiguous requirements. | ||||||
|
|
||||||
| ## When NOT to Use This Tool | ||||||
| If the request is already clear, small, and low-risk, you may execute directly without entering plan mode. Do not make speculative small edits before you have thought the change through. | ||||||
| Do not use this tool just because a task involves planning, is complex, or requires investigation. In the current mode, you can still think, inspect files, ask clarifying questions, and present a plan without switching modes. | ||||||
|
|
||||||
| ## Important | ||||||
| Do NOT use this tool if the user has explicitly asked you not to use plan mode.`; | ||||||
| If plan mode seems helpful but the user has not asked for it, ask first. Do NOT use this tool if the user has explicitly asked you not to use plan mode.`; | ||||||
|
|
||||||
| const enterPlanModeToolSchemaData: FunctionDeclaration = { | ||||||
| name: 'enter_plan_mode', | ||||||
|
|
@@ -154,7 +154,7 @@ export class EnterPlanModeTool extends BaseDeclarativeTool< | |||||
| >, | ||||||
| true, // isOutputMarkdown | ||||||
| false, // canUpdateOutput | ||||||
| false, // shouldDefer — always visible so the model can enter plan mode | ||||||
| false, // shouldDefer — always visible so explicit plan-mode requests work | ||||||
| false, // alwaysLoad | ||||||
| 'plan mode enter start', | ||||||
| ); | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] The test asserts the new opt-in wording is present and the old wording is removed, but does not assert
expect(prompt).toContain('has already enabled it')for the "already enabled" qualifier. This clause covers users who setapprovalMode=planin config — it could be removed from the prompt without the test catching it.— qwen3.7-max via Qwen Code /review