Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 21 additions & 23 deletions src/components/ChatModeSelector.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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">
Copy link
Contributor

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 SelectItem is now visible to all users, but the backend handler handleLocalAgentStream in local_agent_handler.ts:266 has a Pro-only guard:

if (!readOnly && !isDyadProEnabled(settings) && !isBasicAgentMode(settings)) {
    // returns error: "Agent v2 requires Dyad Pro."
}

For non-Pro users selecting Plan mode: readOnly = false, isDyadProEnabled = false, isBasicAgentMode = false → guard fires → error.

The planModeOnly parameter 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.

Suggested change
<SelectItem value="plan">
<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>

Note: The UI change here is correct. The fix needs to be in local_agent_handler.ts:266 — add !planModeOnly to the guard:

if (!readOnly && !planModeOnly && !isDyadProEnabled(settings) && !isBasicAgentMode(settings)) {

<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>
Comment on lines 130 to +153
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There's some structural duplication in how the SelectItem content is rendered for different chat modes. To improve maintainability and reduce repetition, you could extract this into a dedicated component. This would make the list of modes cleaner and easier to manage, especially as more modes might be added in the future.

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">
Expand Down
7 changes: 6 additions & 1 deletion src/pro/main/ipc/handlers/local_agent/local_agent_handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,12 @@ export async function handleLocalAgentStream(
// Check Pro status or Basic Agent mode
// Basic Agent mode allows non-Pro users with quota (quota check is done in chat_stream_handlers)
// Read-only mode (ask mode) is allowed for all users without Pro
if (!readOnly && !isDyadProEnabled(settings) && !isBasicAgentMode(settings)) {
if (
!readOnly &&
!planModeOnly &&
!isDyadProEnabled(settings) &&
!isBasicAgentMode(settings)
) {
safeSend(event.sender, "chat:response:error", {
chatId: req.chatId,
error:
Expand Down
12 changes: 9 additions & 3 deletions src/pro/main/ipc/handlers/local_agent/tools/exit_plan.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 z.literal(true) rejected false at Zod schema validation with a clean, structured error. The new z.boolean() accepts false through validation, then throws a generic Error("User must confirm the plan before exiting plan mode") in execute(). This thrown error may not be caught or surfaced to the user as cleanly as a Zod validation failure would be.

If the intent is to allow the LLM to explicitly pass false to indicate rejection, consider keeping z.literal(true) and adding a separate rejection_reason field, or handle false gracefully in execute() instead of throwing.

),
Comment on lines 8 to +13
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Prompt To Fix With AI
This 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 confirmation field schema was changed from z.literal(true) to z.boolean(). This alters the JSON schema sent to the LLM: previously it was {"type": "boolean", "const": true}, now it is {"type": "boolean"}. This tells the LLM that false is a valid input value.

Root Cause and Impact

With z.literal(true), the LLM could only ever pass true for the confirmation field — the schema itself prevented any other value. With z.boolean(), the LLM might reason "the user hasn't accepted the plan yet, so I'll call exit_plan with confirmation: false" since the schema now advertises this as valid.

When confirmation is false:

  1. buildXml at src/pro/main/ipc/handlers/local_agent/tools/exit_plan.ts:49-50 returns undefined (no UI feedback)
  2. execute at line 56-58 throws "User must confirm the plan before exiting plan mode"
  3. The error is caught by the wrapper in src/pro/main/ipc/handlers/local_agent/tool_definitions.ts:379-386 and rendered as an error XML tag to the user

This wastes one of the 25 allowed tool steps (stepCountIs(25) at local_agent_handler.ts:530), produces a confusing error message for the user, and is a regression from the previous schema that made this impossible at the schema level. The original z.literal(true) was the correct approach — it communicated to the LLM that this tool should only be called when confirmation is granted.

Suggested change
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.",
),
});
.literal(true)
.describe(
"Must be true to confirm the user has accepted the plan.",
),
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


const DESCRIPTION = `
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 exit_plan fires successfully, the downstream flow in usePlanEvents.ts:117-118 switches selectedChatMode to "local-agent", creates a NEW chat, and sends /implement-plan=<slug>. This routes through Basic Agent, which checks quota:

  • Quota available: Works, but silently consumes 1 of the user's 5 daily free messages with no warning.
  • Quota exhausted: User gets FREE_AGENT_QUOTA_EXCEEDED error. They've been navigated to a new empty chat in local-agent mode with no plan context. The plan was saved to .dyad/plans/ but the user has no obvious recovery path.

💡 Suggestion: Check quota BEFORE triggering plan:exit and warn the user, or keep the user in the original plan chat when implementation fails with a message explaining they need quota.

Expand Down
Loading