Add planning_questionnaire to the agent mode#2566
Add planning_questionnaire to the agent mode#2566azizmejri1 wants to merge 3 commits intodyad-sh:mainfrom
Conversation
|
@BugBot run |
Summary of ChangesHello @azizmejri1, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the agent's ability to interact with users by integrating a crucial clarification step into its core workflow. By making the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully extends the planning_questionnaire tool to be available in the standard agent mode, not just in plan mode. This allows the agent to proactively ask clarifying questions before starting implementation, which should improve its performance on ambiguous tasks. The changes are well-executed across the board: the agent's stopping condition is updated, tool definitions are refactored for clarity, and the system prompt is modified to instruct the agent on this new workflow step. The implementation is clean and logical. I have no specific comments as the changes are solid.
🔍 Multi-Agent Code ReviewFound 5 issue(s) flagged by consensus of 3 independent reviewers. Summary
Issues to Address
🟢 Low Priority Issues (2 items)
See inline comments for details on MEDIUM issues. Generated by multi-agent consensus review (3 agents, 2+ agreement threshold) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * Tools that should ONLY be available in plan mode (excluded from normal agent mode). | ||
| * Note: planning_questionnaire is intentionally omitted so it's available in both modes. | ||
| */ | ||
| const PLAN_MODE_ONLY_TOOLS = new Set(["write_plan", "exit_plan"]); |
There was a problem hiding this comment.
🟡 MEDIUM — Fragile relationship between two sets (3/3 agents flagged)
PLANNING_SPECIFIC_TOOLS and PLAN_MODE_ONLY_TOOLS differ by exactly one member (planning_questionnaire). If a future developer adds a new planning tool to one set but forgets the other, the dual-filter logic will silently misbehave.
Consider deriving one from the other programmatically:
| const PLAN_MODE_ONLY_TOOLS = new Set(["write_plan", "exit_plan"]); | |
| const PLAN_MODE_ONLY_TOOLS = new Set([...PLANNING_SPECIFIC_TOOLS].filter(t => t !== "planning_questionnaire")); |
This eliminates the risk of the two sets drifting apart.
src/prompts/local_agent_prompt.ts
Outdated
| 3. **Implement:** Use the available tools (e.g., \`edit_file\`, \`write_file\`, ...) to act on the plan, strictly adhering to the project's established conventions. When debugging, add targeted console.log statements to trace data flow and identify root causes. **Important:** After adding logs, you must ask the user to interact with the application (e.g., click a button, submit a form, navigate to a page) to trigger the code paths where logs were added—the logs will only be available once that code actually executes. | ||
| 4. **Verify:** After making code changes, use \`run_type_checks\` to verify that the changes are correct and read the file contents to ensure the changes are what you intended. | ||
| 5. **Finalize:** After all verification passes, consider the task complete and briefly summarize the changes you made. | ||
| 2. **Clarify (REQUIRED):** Before writing any code, you MUST call the \`planning_questionnaire\` tool to ask the user 1-3 focused questions about unclear or ambiguous aspects of their request. After calling the questionnaire, STOP immediately and wait for the user's responses — do NOT proceed to the Plan step until you receive their answers. Only skip this step if the user's request is completely unambiguous . |
There was a problem hiding this comment.
🟡 MEDIUM — Clarify step too aggressive for simple requests (3/3 agents flagged)
The MUST call + stopWhen halt creates a mandatory user round-trip for every request, including trivial ones like "fix the typo on line 5" or "change button color to blue". The "completely unambiguous" escape hatch is subjective and most models will err on the side of calling the tool.
Two concerns:
- UX friction: Simple follow-up messages in ongoing conversations will also trigger the questionnaire since this fires on every turn, not just the first.
- Contradictory wording:
(REQUIRED)label conflicts with "Only skip this step if..." — consider softening toSHOULDor providing clearer skip criteria (e.g., "Skip for simple, well-defined changes like fixing a typo, renaming a variable, or changing a color").
src/prompts/local_agent_prompt.ts
Outdated
| 3. **Implement:** Use the available tools (e.g., \`edit_file\`, \`write_file\`, ...) to act on the plan, strictly adhering to the project's established conventions. When debugging, add targeted console.log statements to trace data flow and identify root causes. **Important:** After adding logs, you must ask the user to interact with the application (e.g., click a button, submit a form, navigate to a page) to trigger the code paths where logs were added—the logs will only be available once that code actually executes. | ||
| 4. **Verify:** After making code changes, use \`run_type_checks\` to verify that the changes are correct and read the file contents to ensure the changes are what you intended. | ||
| 5. **Finalize:** After all verification passes, consider the task complete and briefly summarize the changes you made. | ||
| 2. **Clarify (REQUIRED):** Before writing any code, you MUST call the \`planning_questionnaire\` tool to ask the user 1-3 focused questions about unclear or ambiguous aspects of their request. After calling the questionnaire, STOP immediately and wait for the user's responses — do NOT proceed to the Plan step until you receive their answers. Only skip this step if the user's request is completely unambiguous . |
There was a problem hiding this comment.
🟡 MEDIUM — Basic agent mode inconsistency (2/3 agents flagged)
The tool filtering change in tool_definitions.ts makes planning_questionnaire available in basic agent mode too (it's no longer in PLAN_MODE_ONLY_TOOLS). However, the Clarify step is only added to PRO_DEVELOPMENT_WORKFLOW_BLOCK, not BASIC_DEVELOPMENT_WORKFLOW_BLOCK.
This means basic-mode agents have the tool available but no prompt guidance to use it. If a model calls it anyway, the stopWhen logic will halt the loop — which could create a confusing UX with no workflow context.
Consider either:
- Adding the Clarify step to
BASIC_DEVELOPMENT_WORKFLOW_BLOCKas well, or - Filtering out
planning_questionnairein basic agent mode
🎭 Playwright Test Results❌ Some tests failed
Summary: 662 passed, 7 failed, 10 flaky, 218 skipped Failed Tests🍎 macOS
🪟 Windows
📋 Test Commands (macOS)Copy and paste to re-run failing tests locally: export PLAYWRIGHT_HTML_OPEN=never
# context_compaction.spec.ts > local-agent - context compaction triggers and shows summary
# Expected: Error: expect(string).toMatchSnapshot(expected) failed
npm run e2e e2e-tests/context_compaction.spec.ts -- -g "local-agent - context compaction triggers and shows summary" --update-snapshots
# context_manage.spec.ts > manage context - exclude paths
# Expected: TimeoutError: locator.click: Timeout 30000ms exceeded.
npm run e2e e2e-tests/context_manage.spec.ts -- -g "manage context - exclude paths"
# local_agent_auto.spec.ts > local-agent - auto model
# Expected: Error: expect(string).toMatchSnapshot(expected) failed
npm run e2e e2e-tests/local_agent_auto.spec.ts -- -g "local-agent - auto model" --update-snapshots
# local_agent_basic.spec.ts > local-agent - dump request
# Expected: Error: expect(string).toMatchSnapshot(expected) failed
npm run e2e e2e-tests/local_agent_basic.spec.ts -- -g "local-agent - dump request" --update-snapshots
# theme_selection.spec.ts > theme selection - app-specific theme is persisted
# Expected: TimeoutError: locator.click: Timeout 30000ms exceeded.
npm run e2e e2e-tests/theme_selection.spec.ts -- -g "theme selection - app-specific theme is persisted"
|
|
@BugBot run |
azizmejri1
force-pushed
the
questionnaire-agent-mode
branch
from
4104006 to
3a277ec
Compare
|
@BugBot run |
|
|
||
| const DESCRIPTION = ` | ||
| Present a structured questionnaire to gather requirements from the user during the planning phase. | ||
| Present a structured questionnaire to gather requirements from the user . |
There was a problem hiding this comment.
Severity: LOW | Category: microcopy
There is a trailing space before the period in the description string:
Present a structured questionnaire to gather requirements from the user .
This trailing space before the period is a typo from removing "during the planning phase". While users don't see this description directly (it's for the LLM), it's a minor blemish.
| Present a structured questionnaire to gather requirements from the user . | |
| Present a structured questionnaire to gather requirements from the user. |
src/prompts/local_agent_prompt.ts
Outdated
| 2. **Clarify (when request is vague or ambiguous):** You MUST call the \`planning_questionnaire\` tool to ask the user 1-3 focused questions when the request is vague, broad, or leaves important decisions unspecified. After calling the questionnaire, STOP immediately and wait for the user's responses — do NOT proceed to the Plan step until you receive their answers. Only skip this step when the request is specific and actionable enough to implement without assumptions. | ||
| - Vague requests — MUST call \`planning_questionnaire\`: "make it look better", "add authentication", "improve performance", "build a dashboard", "add a settings page", "build me a habit streak tracker" | ||
| - Clear requests — skip clarification: "change the button color to blue", "fix the typo in the header", "add a loading spinner to the submit button", "rename the variable from x to count" |
There was a problem hiding this comment.
Severity: MEDIUM | Category: interaction
The "Clarify" step may be overly aggressive for experienced users, and vague/clear boundary is fuzzy.
The prompt says the agent "MUST call" the questionnaire for requests like "add authentication", "build a dashboard", or "build me a habit streak tracker". Many experienced users deliberately write brief prompts because they trust the AI to make reasonable decisions -- being forced through a multi-question wizard every time will feel patronizing and slow down their workflow.
For example, "build me a habit streak tracker" is listed as vague, but it's arguably quite specific about what to build. Users may find the agent's threshold for "vague" to be frustratingly low.
Additionally, there is no way for the user to dismiss the questionnaire without answering it (looking at QuestionnaireInput.tsx, there is no "Skip" or "X" button). If the user just wants to type a follow-up message normally, the questionnaire persists above the input with no close affordance. This is more critical now that questionnaires can appear in every normal conversation, not just opt-in plan mode.
Suggestions:
- Soften "MUST call" to "SHOULD call" so the agent uses judgment rather than treating this as mandatory for a broad set of requests.
- Add a "Skip" / dismiss button to the questionnaire UI so users who don't want to answer can close it.
- Consider a user setting to disable clarification questions entirely.
| /** | ||
| * Tools that should ONLY be available in plan mode (excluded from normal agent mode). | ||
| * Note: planning_questionnaire is intentionally omitted so it's available in both modes. | ||
| */ | ||
| const PLAN_MODE_ONLY_TOOLS = new Set(["write_plan", "exit_plan"]); |
There was a problem hiding this comment.
Severity: HIGH | Category: consistency
The Basic Agent mode (free tier) gets planning_questionnaire in its toolset but no prompt guidance to use it.
PLAN_MODE_ONLY_TOOLS only excludes write_plan and exit_plan, so planning_questionnaire is now available to all non-plan modes -- including basic agent mode. However, the BASIC_DEVELOPMENT_WORKFLOW_BLOCK in local_agent_prompt.ts was not updated to include a "Clarify" step. Basic agent users will have the tool but the system prompt never tells the agent to use it.
This creates an inconsistent experience:
- Pro users: Agent actively asks clarifying questions for vague requests using the structured questionnaire UI.
- Basic (free) users: Agent has the tool but no prompt guidance, so it will either never use it or use it unpredictably.
Either:
- Add a parallel "Clarify" step to
BASIC_DEVELOPMENT_WORKFLOW_BLOCK, or - Add
"planning_questionnaire"toPLAN_MODE_ONLY_TOOLSif this feature is intentionally Pro-only, then gate it separately for normal-agent-pro vs. normal-agent-basic.
| // normal agent mode. Without this, some models (e.g. Gemini Pro 3) | ||
| // ignore the prompt-level "STOP" instruction and keep calling tools | ||
| // in a loop. | ||
| hasToolCall(planningQuestionnaireTool.name), |
There was a problem hiding this comment.
Severity: MEDIUM | Category: interaction
Stale questionnaire UI when the user cancels or switches chats.
The pendingQuestionnaireAtom is only cleared when the user submits the questionnaire (setQuestionnaire(null) in QuestionnaireInput.tsx). If the user:
- Gets a questionnaire in agent mode
- Cancels the stream (stop button) or switches to a different chat
- Returns to the original chat
...the questionnaire will still be showing (the atom is global, keyed by chatId). While the existing questionnaire.chatId !== chatId guard in the component hides it when on a different chat, canceling the stream on the same chat leaves the questionnaire visible with no way to dismiss it (no "Skip" or "X" button exists).
Consider clearing pendingQuestionnaireAtom when a stream is canceled, similar to how clearPendingConsentsForChat is called on abort.
🔍 Dyadbot Code Review SummaryFound 1 new issue(s) flagged by 3 independent reviewers. Summary
Issues to Address
🟢 Low Priority Issues (5 items — not consensus, single-agent only)
See inline comments for details. Generated by Dyadbot multi-agent code review (3 independent reviewers with consensus voting) |
|
|
||
| const DESCRIPTION = ` | ||
| Present a structured questionnaire to gather requirements from the user during the planning phase. | ||
| Present a structured questionnaire to gather requirements from the user . |
There was a problem hiding this comment.
🟡 MEDIUM — Trailing space before period in tool description (3/3 agents flagged)
The description reads "...from the user ." with a stray space before the period — an artifact of removing "during the planning phase" without cleaning up whitespace. This string is sent to the LLM as the tool description, and sloppy formatting in tool descriptions can subtly affect model behavior since models are sensitive to description quality.
| Present a structured questionnaire to gather requirements from the user . | |
| Present a structured questionnaire to gather requirements from the user. |
github-actions
bot
added
the
needs-human:review-issue
|
@BugBot run |
|
All contributors have signed the CLA ✍️ ✅ |
🔍 Dyadbot Code Review SummaryFound 7 consensus issue(s) flagged by 2+ of 3 independent reviewers. Summary
Issues to Address
🟢 Low Priority Issues (5 items)
See inline comments for details on MEDIUM issues. Generated by Dyadbot multi-agent code review (3 agents, consensus voting) |
| // Note: The "incomplete todo(s)" substring is used as a detection marker by test | ||
| // infrastructure in testing/fake-llm-server/ (chatCompletionHandler.ts and | ||
| // localAgentHandler.ts). Update those files if this text changes. | ||
| return `You have ${incompleteTodos.length} incomplete todo(s). Please continue and complete them:\n\n${todoList}`; |
There was a problem hiding this comment.
🟡 MEDIUM | Consensus: 2/3 reviewers
Magic string coupling between production and test code
The substring "incomplete todo(s)" is used as a detection marker across three files (prepare_step_utils.ts, chatCompletionHandler.ts:51, localAgentHandler.ts:55). The comment warns to update those files if text changes, but this is a fragile contract enforced only by comments. If the text changes, the test infrastructure will silently break — the fake LLM server won't detect todo reminders and multi-pass fixtures will fail.
Suggestion: Extract the magic substring into a shared constant (e.g., TODO_REMINDER_MARKER in a shared types/constants file that both production and test code can import). This turns a runtime failure into a compile-time guarantee.
Code Health ReviewMEDIUM - Stale comment in
|
|
|
||
| const DESCRIPTION = ` | ||
| Present a structured questionnaire to gather requirements from the user during the planning phase. | ||
| Present a structured questionnaire to gather requirements from the user . |
There was a problem hiding this comment.
Nit: trailing space before the period
There's an extra space before the period in "from the user ." -- should be "from the user.".
| Present a structured questionnaire to gather requirements from the user . | |
| Present a structured questionnaire to gather requirements from the user. |
src/prompts/local_agent_prompt.ts
Outdated
| 3. **Implement:** Use the available tools (e.g., \`edit_file\`, \`write_file\`, ...) to act on the plan, strictly adhering to the project's established conventions. When debugging, add targeted console.log statements to trace data flow and identify root causes. **Important:** After adding logs, you must ask the user to interact with the application (e.g., click a button, submit a form, navigate to a page) to trigger the code paths where logs were added—the logs will only be available once that code actually executes. | ||
| 4. **Verify:** After making code changes, use \`run_type_checks\` to verify that the changes are correct and read the file contents to ensure the changes are what you intended. | ||
| 5. **Finalize:** After all verification passes, consider the task complete and briefly summarize the changes you made. | ||
| 2. **Clarify (REQUIRED):** Before writing any code, you MUST call the \`planning_questionnaire\` tool to ask the user 1-3 focused questions about unclear or ambiguous aspects of their request. After calling the questionnaire, STOP immediately and wait for the user's responses — do NOT proceed to the Plan step until you receive their answers. Only skip this step if the user's request is completely unambiguous . |
There was a problem hiding this comment.
UX concern (MEDIUM): Trailing space before period in the prompt text
Minor, but "Only skip this step if the user's request is completely unambiguous ." has a trailing space before the period. This gets sent to the LLM and could be perceived as sloppy, and in edge cases might subtly affect model behavior.
| 2. **Clarify (REQUIRED):** Before writing any code, you MUST call the \`planning_questionnaire\` tool to ask the user 1-3 focused questions about unclear or ambiguous aspects of their request. After calling the questionnaire, STOP immediately and wait for the user's responses — do NOT proceed to the Plan step until you receive their answers. Only skip this step if the user's request is completely unambiguous . | |
| 2. **Clarify (REQUIRED):** Before writing any code, you MUST call the \`planning_questionnaire\` tool to ask the user 1-3 focused questions about unclear or ambiguous aspects of their request. After calling the questionnaire, STOP immediately and wait for the user's responses — do NOT proceed to the Plan step until you receive their answers. Only skip this step if the user's request is completely unambiguous. |
| /** | ||
| * Tools that should ONLY be available in plan mode (excluded from normal agent mode). | ||
| * Note: planning_questionnaire is intentionally omitted so it's available in both modes. | ||
| */ | ||
| const PLAN_MODE_ONLY_TOOLS = new Set(["write_plan", "exit_plan"]); |
There was a problem hiding this comment.
UX concern (MEDIUM): Basic Agent Mode gets the tool but not the prompt guidance
PLAN_MODE_ONLY_TOOLS omits planning_questionnaire, making it available in all non-plan modes, including Basic Agent Mode. However, the BASIC_DEVELOPMENT_WORKFLOW_BLOCK prompt (at line 128 of local_agent_prompt.ts) was not updated with a "Clarify" step. This creates an inconsistency:
- Pro Agent Mode: Mandatory Clarify step in the prompt + tool available = users see questionnaires regularly
- Basic Agent Mode: No Clarify step in the prompt + tool available = the model may randomly call it, surprising users, or never call it at all
Consider either adding a similar Clarify step to the Basic prompt, or filtering planning_questionnaire out of Basic Agent Mode so it doesn't appear unexpectedly.
Additionally, there is no way for users to dismiss or skip a questionnaire once it appears. The QuestionnaireInput component has no "Skip" or "Just proceed" button. The user must answer and submit, or collapse the UI and type a free-form message (which is non-obvious). Since questionnaires are now more frequent in normal mode, a lightweight skip affordance would improve the experience for users who already know what they want.
🔍 Dyadbot Code Review SummaryRe-reviewed with 3 independent agents (Correctness Expert, Code Health Expert, UX Wizard). 0 new issue(s) to report — all consensus findings were already covered by existing comments. Consensus Issues (already commented)
🟢 Low Priority Issues (2 items — already commented)
💡 Single-agent findings (did not reach consensus, for reference only)
Generated by Dyadbot multi-agent code review (3 agents, consensus voting) |
|
@BugBot run |
|
In addition to adding planning_questionnaire to agent mode, I addressed issues observed in some models like Kimi 2.5 and GLM:
|
src/prompts/local_agent_prompt.ts
Outdated
| - The request is vague or open-ended (e.g. "Add authentication", "Make it look better", "Improve performance") | ||
| - There are multiple reasonable interpretations of what the user wants | ||
| When any of the above apply, do NOT output any text explanation — just immediately call the \`planning_questionnaire\` tool. | ||
| **Skip the questionnaire ONLY when** the request is a specific, concrete change like: "Add a dark mode toggle to the navbar", "Fix the login button — it's not calling the API", "Change the primary color from blue to green"=3. **Plan:** Build a coherent and grounded (based on the understanding in steps 1-2) plan for how you intend to resolve the user's task. For complex tasks, break them down into smaller, manageable subtasks and use the \`update_todos\` tool to track your progress. Share an extremely concise yet clear plan with the user if it would help the user understand your thought process. |
There was a problem hiding this comment.
Bug (MEDIUM): Broken step numbering -- =3. merges step 3 into step 2's final sentence.
The text reads:
"Change the primary color from blue to green"=3. **Plan:** Build a coherent...
It looks like a newline was lost during editing, so = and the step number 3. ended up concatenated to the end of step 2. The LLM will receive a garbled workflow where steps 2 and 3 appear on the same line, making step 3 ("Plan") invisible as a distinct workflow step.
| **Skip the questionnaire ONLY when** the request is a specific, concrete change like: "Add a dark mode toggle to the navbar", "Fix the login button — it's not calling the API", "Change the primary color from blue to green"=3. **Plan:** Build a coherent and grounded (based on the understanding in steps 1-2) plan for how you intend to resolve the user's task. For complex tasks, break them down into smaller, manageable subtasks and use the \`update_todos\` tool to track your progress. Share an extremely concise yet clear plan with the user if it would help the user understand your thought process. | |
| **Skip the questionnaire ONLY when** the request is a specific, concrete change like: "Add a dark mode toggle to the navbar", "Fix the login button — it's not calling the API", "Change the primary color from blue to green" | |
| 3. **Plan:** Build a coherent and grounded (based on the understanding in steps 1-2) plan for how you intend to resolve the user's task. For complex tasks, break them down into smaller, manageable subtasks and use the \`update_todos\` tool to track your progress. Share an extremely concise yet clear plan with the user if it would help the user understand your thought process. |
| type: z.literal("text"), | ||
| type: z.enum(["text", "radio", "checkbox"]), | ||
| question: z.string(), | ||
| options: z.array(z.string()).min(1).optional(), |
There was a problem hiding this comment.
Bug (MEDIUM): options is now optional for radio/checkbox types, but the frontend has no fallback -- user gets stuck.
The old discriminated union required options (with .min(1)) for radio and checkbox types. The new unified schema makes options optional for all types. If the LLM omits options on a radio or checkbox question, both Zod schemas (here and in planning_questionnaire.ts) will happily accept it.
On the frontend, QuestionnaireInput.tsx guards radio/checkbox rendering with currentQuestion.options && (lines 243, 315). When options is undefined, no input widget renders at all. The user sees a required question label with nothing to interact with, and the "Next" button stays disabled (hasValidAnswer() returns false), deadlocking the questionnaire.
Suggested fix -- use Zod's .refine() or .superRefine() to require options when type is radio or checkbox:
export const QuestionSchema = z.object({
id: z.string(),
type: z.enum(["text", "radio", "checkbox"]),
question: z.string(),
options: z.array(z.string()).min(1).optional(),
required: z.boolean().optional(),
placeholder: z.string().optional(),
}).refine(
(q) => q.type === "text" || (q.options && q.options.length >= 1),
{ message: "options are required for radio and checkbox questions", path: ["options"] },
);Alternatively, as a defensive measure, the frontend could fall back to rendering a text input when options is missing for radio/checkbox types.
|
@BugBot run |
🔍 Dyadbot Code Review SummaryFound 2 new issue(s) flagged by 3 independent reviewers. Summary
Issues to Address
See inline comments for details. Generated by Dyadbot code review |
🔍 Dyadbot Code Review SummaryFound 2 new issue(s) flagged by 3 independent reviewers. Summary
Issues to Address
See inline comments for details. Generated by Dyadbot code review |
| @@ -16,32 +29,8 @@ const BaseQuestionFields = { | |||
| .string() | |||
| .optional() | |||
| .describe("Placeholder text for text inputs"), | |||
| }; | |||
|
|
|||
| const TextQuestionSchema = z.object({ | |||
| ...BaseQuestionFields, | |||
| type: z.literal("text"), | |||
| }); | |||
There was a problem hiding this comment.
🟡 MEDIUM — Duplicate QuestionSchema can drift out of sync (3/3 agents flagged)
This QuestionSchema is a separate definition from the exported QuestionSchema in src/ipc/types/plan.ts:26-33. They're similar but already diverge: this one has .max(3) on options, the IPC types version does not. Since the tool schema is what the LLM sees and the IPC schema is what the frontend validates against, a future change to one that isn't mirrored in the other could cause questions to pass tool validation but fail IPC validation (or vice versa), silently dropping questionnaires.
Consider importing the canonical QuestionSchema from @/ipc/types/plan and extending it with .describe() annotations for the LLM tool definition, ensuring a single source of truth.
| // After the questionnaire is presented, strip all tools so the | ||
| // model is forced to stop instead of continuing to call tools. | ||
| // This replaces hasToolCall() in stopWhen which closed the stream | ||
| // before the frontend could process the questionnaire IPC message. | ||
| if (questionnairePresented) { | ||
| return { ...(result ?? options), toolChoice: "none" as const }; | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM — Post-questionnaire step may emit confusing filler text (2/3 agents flagged)
The toolChoice: "none" approach forces the model to produce one more text-only generation step after the questionnaire is presented. Some models will emit filler like "I'll wait for your responses..." which appears in the chat alongside the questionnaire card. This adds noise — the user sees both the questionnaire UI and a text message telling them to answer it.
The old stopWhen: hasToolCall() prevented any post-questionnaire text (though it had the IPC timing issue this PR fixes). Consider also setting maxTokens: 1 on the forced-text step to minimize filler, or suppressing text-delta chunks from the UI when questionnairePresented is true.
Additionally, for robustness: the spread copies tools from options into the result. While toolChoice: "none" should prevent tool calls at the API level, setting tools: {} alongside it would provide defense-in-depth against providers that don't fully respect toolChoice: "none".
|
@BugBot run |
Greptile OverviewGreptile SummaryThis PR enables the
The implementation correctly handles the async nature of IPC by setting a flag in Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Agent as Local Agent Handler
participant LLM
participant IPC as IPC Channel
participant Frontend
User->>Agent: Send vague request (e.g. "Build me a todo app")
Agent->>Agent: Workflow Step 1: Understand (explore codebase)
Agent->>LLM: Stream request with planning_questionnaire tool
LLM->>Agent: Tool call: planning_questionnaire
Agent->>Agent: onStepFinish: Set questionnairePresented = true
Agent->>IPC: Send plan:questionnaire event
IPC->>Frontend: Display questionnaire UI
Agent->>LLM: Next step with prepareStep
Agent->>Agent: prepareStep: Apply toolChoice: "none"
LLM->>Agent: Stop (no tools available)
Agent->>Frontend: Complete turn
User->>Frontend: Submit questionnaire answers
Frontend->>Agent: User message with answers
Agent->>Agent: Reset questionnairePresented = false
Agent->>LLM: Continue with clarified requirements
LLM->>Agent: Proceed to Plan step with context
Last reviewed commit: 1655c53 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| type: z.literal("text"), | ||
| type: z.enum(["text", "radio", "checkbox"]), | ||
| question: z.string(), | ||
| options: z.array(z.string()).min(1).optional(), |
There was a problem hiding this comment.
The options field is optional but still has .min(1) validation. This creates a situation where if options is provided, it must have at least one element, but the validation for radio/checkbox questions requiring options happens elsewhere. Consider whether the validation should be more explicit at this schema level.
For text questions, options should be undefined/omitted. For radio/checkbox, it should be required. The current schema allows radio/checkbox questions with undefined options, which would be invalid at runtime.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ipc/types/plan.ts
Line: 30:30
Comment:
The `options` field is optional but still has `.min(1)` validation. This creates a situation where if `options` is provided, it must have at least one element, but the validation for radio/checkbox questions requiring options happens elsewhere. Consider whether the validation should be more explicit at this schema level.
For text questions, `options` should be undefined/omitted. For radio/checkbox, it should be required. The current schema allows radio/checkbox questions with undefined options, which would be invalid at runtime.
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files
Confidence score: 3/5
- Some risk: validation is weakened for multiple-choice questions, which can allow invalid questionnaires (radio/checkbox without options).
src/ipc/types/plan.tsnow makesoptionsoptional for all question types, removing a safeguard for multiple-choice inputs.- Pay close attention to
src/ipc/types/plan.ts,src/pro/main/ipc/handlers/local_agent/tools/planning_questionnaire.ts- schema/type changes allow invalid option configurations.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/ipc/types/plan.ts">
<violation number="1" location="src/ipc/types/plan.ts:26">
P2: `options` is now optional for all question types, which permits `radio`/`checkbox` questions without any options. This weakens validation and can produce invalid questionnaires for multiple-choice inputs. Restore type-specific validation so options are required for radio/checkbox and omitted for text.</violation>
</file>
<file name="src/pro/main/ipc/handlers/local_agent/tools/planning_questionnaire.ts">
<violation number="1" location="src/pro/main/ipc/handlers/local_agent/tools/planning_questionnaire.ts:20">
P2: Question schema no longer enforces options for radio/checkbox and allows options on text questions, so invalid multiple-choice inputs can pass validation. Consider restoring a discriminated union or adding a refinement to require `options` when type is radio/checkbox and forbid it for text.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| export type PlanExitPayload = z.infer<typeof PlanExitSchema>; | ||
|
|
||
| const TextQuestionSchema = z.object({ | ||
| export const QuestionSchema = z.object({ |
There was a problem hiding this comment.
P2: options is now optional for all question types, which permits radio/checkbox questions without any options. This weakens validation and can produce invalid questionnaires for multiple-choice inputs. Restore type-specific validation so options are required for radio/checkbox and omitted for text.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ipc/types/plan.ts, line 26:
<comment>`options` is now optional for all question types, which permits `radio`/`checkbox` questions without any options. This weakens validation and can produce invalid questionnaires for multiple-choice inputs. Restore type-specific validation so options are required for radio/checkbox and omitted for text.</comment>
<file context>
@@ -23,28 +23,15 @@ export const PlanExitSchema = z.object({
export type PlanExitPayload = z.infer<typeof PlanExitSchema>;
-const TextQuestionSchema = z.object({
+export const QuestionSchema = z.object({
id: z.string(),
- type: z.literal("text"),
</file context>
| .array(z.string()) | ||
| .min(1) | ||
| .max(3) | ||
| .optional() |
There was a problem hiding this comment.
P2: Question schema no longer enforces options for radio/checkbox and allows options on text questions, so invalid multiple-choice inputs can pass validation. Consider restoring a discriminated union or adding a refinement to require options when type is radio/checkbox and forbid it for text.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/pro/main/ipc/handlers/local_agent/tools/planning_questionnaire.ts, line 20:
<comment>Question schema no longer enforces options for radio/checkbox and allows options on text questions, so invalid multiple-choice inputs can pass validation. Consider restoring a discriminated union or adding a refinement to require `options` when type is radio/checkbox and forbid it for text.</comment>
<file context>
@@ -5,9 +5,22 @@ import { safeSend } from "@/ipc/utils/safe_sender";
+ .array(z.string())
+ .min(1)
+ .max(3)
+ .optional()
+ .describe(
+ "Options for radio/checkbox questions. Keep to max 3 — users can always provide a custom answer via the free-form text input. Omit for text questions.",
</file context>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 Todo follow-up pass can run after questionnaire presentation, violating "stop and wait" semantics
After the planning_questionnaire tool is called in normal agent mode, the model is forced to stop by setting toolChoice: "none" in prepareStep. This causes the model to emit a text-only response, which sets passEndedWithText = true. After the stream finishes, shouldRunTodoFollowUpPass at line 881 is evaluated — it checks !readOnly && !planModeOnly && passEndedWithText && hasIncompleteTodos(todos) && todoFollowUpLoops < maxTodoFollowUpLoops. In normal agent mode (planModeOnly = false), if incomplete todos exist, this condition is satisfied, and a new generation pass is started.
Root Cause
The shouldRunTodoFollowUpPass function at src/pro/main/ipc/handlers/local_agent/local_agent_handler.ts:1034-1057 has no awareness of whether a questionnaire was just presented. The questionnairePresented flag is scoped inside the while loop and reset to false at the start of each pass (line 532), so it cannot be checked by shouldRunTodoFollowUpPass.
When the questionnaire is used in normal agent mode (the new behavior enabled by this PR), the agent may have already created todos via update_todos before deciding to ask clarifying questions. If any of those todos are incomplete, the follow-up pass fires, injecting a todo reminder and continuing generation — directly contradicting the questionnaire's instruction to "STOP HERE and wait for the user to respond."
Impact: The agent continues acting after presenting a questionnaire instead of yielding control to the user, potentially making unwanted changes before the user has answered the clarifying questions.
(Refers to lines 880-891)
Prompt for agents
Propagate the questionnairePresented flag so that shouldRunTodoFollowUpPass can check it. One approach: declare questionnairePresented before the while loop (or hoist it out), and add a questionnairePresented parameter to shouldRunTodoFollowUpPass that causes it to return false. For example, add questionnairePresented to the params object at line 881 and add !questionnairePresented to the return condition at line 1050. Make sure questionnairePresented is reset at the top of each while-loop iteration so it only affects the pass in which the questionnaire was actually presented.
Was this helpful? React with 👍 or 👎 to provide feedback.
| export const QuestionSchema = z.object({ | ||
| id: z.string(), | ||
| type: z.literal("text"), | ||
| type: z.enum(["text", "radio", "checkbox"]), | ||
| question: z.string(), | ||
| options: z.array(z.string()).min(1).optional(), | ||
| required: z.boolean().optional(), | ||
| placeholder: z.string().optional(), | ||
| }); |
There was a problem hiding this comment.
🟡 Schema refactor allows radio/checkbox questions without options, weakening validation
The QuestionSchema in plan.ts was refactored from a discriminated union to a single flat object. The old union enforced that options was required (with .min(1)) for radio/checkbox types, while omitting it for text types. The new schema makes options optional for all types (z.array(z.string()).min(1).optional() at line 30).
Detailed Explanation
The same weakening exists in the tool input schema at src/pro/main/ipc/handlers/local_agent/tools/planning_questionnaire.ts:17-22. This means the LLM can generate a radio or checkbox question without any options and it will pass both the tool input validation and the IPC event validation.
The frontend at src/components/chat/QuestionnaireInput.tsx:243 and :315 guards against missing options with currentQuestion.options &&, so it won't crash, but the rendered UI will show only the "Other..." free-form input for a radio/checkbox question — a confusing experience where the user sees a question type that implies predefined choices but none are shown.
Previously, the discriminated union schema (MultipleChoiceQuestionSchema) required options: z.array(z.string()).min(1) for radio/checkbox types, making this invalid state unrepresentable.
Impact: Radio/checkbox questions may render without any selectable options, degrading the questionnaire UX.
Prompt for agents
Add a Zod .refine() or .superRefine() to QuestionSchema to enforce that options is required (and non-empty) when type is 'radio' or 'checkbox'. Apply the same refinement to the local QuestionSchema in src/pro/main/ipc/handlers/local_agent/tools/planning_questionnaire.ts. For example: QuestionSchema.refine(q => q.type === 'text' || (q.options && q.options.length > 0), { message: 'options is required for radio and checkbox question types' }).
Was this helpful? React with 👍 or 👎 to provide feedback.
🔍 Dyadbot Code Review Summary3 independent reviewers analyzed this PR. All consensus issues from this review round have already been commented on in prior reviews. Consensus Issues (already commented)
Non-Consensus Issues Worth NotingThese were flagged by only 1 of 3 agents and do not meet the 2/3 consensus threshold, but may still be worth considering: ℹ️ Single-agent findings (6 items)
No new inline comments posted (all consensus issues already addressed). Generated by Dyadbot code review |
wwwillchen
left a comment
wwwillchen
left a comment
There was a problem hiding this comment.
it seems like there's cases where a model like gemini flash 3 will produce invalid results
{
id:"site_purpose",
question:"What is the main purpose of this site?",
type:"radio",
required:true
},
I'd play around with this and try the following:
- offer much stricter prompting, e.g. say you MUST have multiple options if it s a radio or checkbox question.
- if the model provides 0 choices and radio/checkbox, we can either make it a free-form or throw an error. I'd try playing around and see which one feels better. For example, in the screenshot i took, that's probably not a bad question to turn into a free-form question, but maybe in other cases it becomes nonsensical.
btw, you'll need to rebaseline some of the e2e tests (look at the playwright report comment) which dump out the tool schema
2 participants
Summary by cubic
Enable planning_questionnaire in normal agent mode and enforce a clarify‑before‑plan workflow with explicit examples. Also adds a one‑pass todo follow‑up so the agent completes remaining tasks before stopping.
New Features
Bug Fixes
Written for commit c6a67ea. Summary will update on new commits.