-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat: make Plan mode available for all users #2625
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -128,31 +128,29 @@ export function ChatModeSelector() { | |
| </Tooltip> | ||
| <SelectContent align="start"> | ||
| {isProEnabled && ( | ||
| <> | ||
| <SelectItem value="local-agent"> | ||
| <div className="flex flex-col items-start"> | ||
| <div className="flex items-center gap-1.5"> | ||
| <span className="font-medium">Agent v2</span> | ||
| <NewBadge /> | ||
| </div> | ||
| <span className="text-xs text-muted-foreground"> | ||
| Better at bigger tasks and debugging | ||
| </span> | ||
| </div> | ||
| </SelectItem> | ||
| <SelectItem value="plan"> | ||
| <div className="flex flex-col items-start"> | ||
| <div className="flex items-center gap-1.5"> | ||
| <span className="font-medium">Plan</span> | ||
| <NewBadge /> | ||
| </div> | ||
| <span className="text-xs text-muted-foreground"> | ||
| Design before you build | ||
| </span> | ||
| <SelectItem value="local-agent"> | ||
| <div className="flex flex-col items-start"> | ||
| <div className="flex items-center gap-1.5"> | ||
| <span className="font-medium">Agent v2</span> | ||
| <NewBadge /> | ||
| </div> | ||
| </SelectItem> | ||
| </> | ||
| <span className="text-xs text-muted-foreground"> | ||
| Better at bigger tasks and debugging | ||
| </span> | ||
| </div> | ||
| </SelectItem> | ||
| )} | ||
| <SelectItem value="plan"> | ||
| <div className="flex flex-col items-start"> | ||
| <div className="flex items-center gap-1.5"> | ||
| <span className="font-medium">Plan</span> | ||
| <NewBadge /> | ||
| </div> | ||
| <span className="text-xs text-muted-foreground"> | ||
| Design before you build | ||
| </span> | ||
| </div> | ||
| </SelectItem> | ||
devin-ai-integration[bot] marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
130
to
+153
Contributor
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. There's some structural duplication in how the For example, you could create a helper component: const ChatModeItem = ({
title,
description,
showNewBadge = false,
}: {
title: string;
description: string;
showNewBadge?: boolean;
}) => (
<div className="flex flex-col items-start">
{showNewBadge ? (
<div className="flex items-center gap-1.5">
<span className="font-medium">{title}</span>
<NewBadge />
</div>
) : (
<span className="font-medium">{title}</span>
)}
<span className="text-xs text-muted-foreground">{description}</span>
</div>
);And then use it like this for the common cases: <SelectItem value="local-agent">
<ChatModeItem title="Agent v2" description="Better at bigger tasks and debugging" showNewBadge />
</SelectItem>
<SelectItem value="plan">
<ChatModeItem title="Plan" description="Design before you build" showNewBadge />
</SelectItem>
<SelectItem value="build">
<ChatModeItem title="Build" description="Generate and edit code" />
</SelectItem>This would simplify the main component's render method and make it more declarative. |
||
| {!isProEnabled && ( | ||
| <SelectItem value="local-agent" disabled={isQuotaExceeded}> | ||
| <div className="flex flex-col items-start"> | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -7,8 +7,10 @@ const logger = log.scope("exit_plan"); | |||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const exitPlanSchema = z.object({ | ||||||||||||||||||||||||||
| confirmation: z | ||||||||||||||||||||||||||
| .literal(true) | ||||||||||||||||||||||||||
| .describe("Must be true to confirm the user has accepted the plan"), | ||||||||||||||||||||||||||
| .boolean() | ||||||||||||||||||||||||||
| .describe( | ||||||||||||||||||||||||||
| "Whether the user has accepted the plan. Must be true to proceed.", | ||||||||||||||||||||||||||
|
Comment on lines
9
to
+12
Contributor
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. 🟡 MEDIUM | contract-violation | Found by: Correctness, Endorsed by: Code Health, UX Schema validation weakened from z.literal(true) to z.boolean() The original If the intent is to allow the LLM to explicitly pass |
||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||
|
Comment on lines
8
to
+13
Contributor
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. Schema allows invalid calls
If the intention is still “only allow true”, keep the schema as Prompt To Fix With AIThis is a comment left during a code review.
Path: src/pro/main/ipc/handlers/local_agent/tools/exit_plan.ts
Line: 8:13
Comment:
**Schema allows invalid calls**
`confirmation` used to be `z.literal(true)`, which prevented the model from calling `exit_plan` unless it set `confirmation: true`. Changing this to `z.boolean()` means the model can now (and will occasionally) emit `{"confirmation": false}` tool calls; those will pass schema validation but then throw at runtime in `execute()`. That new failure mode can interrupt a plan-mode tool loop and surface as an avoidable tool error.
If the intention is still “only allow true”, keep the schema as `z.literal(true)` (or use `.refine(v => v === true)` so invalid inputs are rejected at validation time rather than during execution).
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
Comment on lines
9
to
14
Contributor
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. 🟡 exit_plan schema weakened from z.literal(true) to z.boolean() allows LLM to call tool with confirmation: false The Root Cause and ImpactWith When
This wastes one of the 25 allowed tool steps (
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. |
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const DESCRIPTION = ` | ||||||||||||||||||||||||||
|
|
@@ -50,7 +52,11 @@ export const exitPlanTool: ToolDefinition<z.infer<typeof exitPlanSchema>> = { | |||||||||||||||||||||||||
| return `<dyad-exit-plan></dyad-exit-plan>`; | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| execute: async (_args, ctx: AgentContext) => { | ||||||||||||||||||||||||||
| execute: async (args, ctx: AgentContext) => { | ||||||||||||||||||||||||||
| if (!args.confirmation) { | ||||||||||||||||||||||||||
| throw new Error("User must confirm the plan before exiting plan mode"); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| logger.log("Exiting plan mode, transitioning to implementation"); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| safeSend(ctx.event.sender, "plan:exit", { | ||||||||||||||||||||||||||
|
Comment on lines
+55
to
62
Contributor
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. 🔴 HIGH | user-facing-behavior | Found by: UX, Endorsed by: Correctness, Code Health Non-Pro user with exhausted quota gets stranded after accepting plan When a non-Pro user accepts a plan and
💡 Suggestion: Check quota BEFORE triggering |
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
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.
🔴 HIGH | logic | Found by: Correctness Expert, Verified by: Team Lead
Plan mode is blocked by Pro-only backend guard for non-Pro users
The Plan
SelectItemis now visible to all users, but the backend handlerhandleLocalAgentStreaminlocal_agent_handler.ts:266has a Pro-only guard:For non-Pro users selecting Plan mode:
readOnly= false,isDyadProEnabled= false,isBasicAgentMode= false → guard fires → error.The
planModeOnlyparameter is not checked by this guard. Non-Pro users will see Plan mode in the UI but get an error when they try to use it.