Conversation
|
| const questionItemSchema = z.object({ | ||
| id: z | ||
| .string() | ||
| .optional() | ||
| .describe("Optional unique identifier for this question (e.g. 'choice_format' or 'framework')"), |
There was a problem hiding this comment.
Duplicate question IDs break answers
The schema allows repeated question IDs, including an explicit ID that can collide with a generated q_<index> fallback. The card then uses each ID as both a Svelte list key and the key for its answer state. A call with duplicate IDs can fail to render or make separate questions share selections, preventing the user from answering them independently. Please enforce uniqueness after fallback IDs are assigned.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agent/tools/askQuestion.ts
Line: 10-14
Comment:
**Duplicate question IDs break answers**
The schema allows repeated question IDs, including an explicit ID that can collide with a generated `q_<index>` fallback. The card then uses each ID as both a Svelte list key and the key for its answer state. A call with duplicate IDs can fail to render or make separate questions share selections, preventing the user from answering them independently. Please enforce uniqueness after fallback IDs are assigned.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Leo310
left a comment
There was a problem hiding this comment.
Thanks for this, @Klabundo. A structured way for the agent to ask clarifying questions is something we want. I tried it in a test vault, and a single question works. Before merging, though, I'd like to change the design and fix a few bugs.
Design
1. Make it a plain built-in tool, not a skill special case. "Ask when you're unsure" is how every agent should behave, not a domain capability, so it doesn't need a skill. Right now it bypasses the allowed-tools rule through toolId === "ask_question" special cases in isToolBound / buildToolsForAgent / ToolsModal. Instead, bind it as an always-available built-in tool that the per-agent toggle can turn off. Put the "when to ask" guidance in a short section of DEFAULT_AGENT_PROMPT, so it's user-editable in the agent note:
- ask only when the answer really changes the outcome
- offer 2–4 concrete options
- don't ask about anything you can look up in the vault
That also lets the duplicated ext.id === "canvas" toggle in AgentEditorModal go away; the Tools modal is enough.
2. Use LangGraph's interrupt() instead of an in-memory promise. This is the root cause of most of the bugs below. PendingQuestionStore holds a promise in memory, so:
- the question and the tool call have to be matched up by hand, which is where the id bugs come from;
- a pending question is lost on reload, thread switch, or restart. The checkpoint just shows a tool call with no answer.
With interrupt({ questions }) inside the tool, the pending question is saved in the thread's checkpoint under the real tool-call id. The card renders whenever the thread has an unanswered interrupt, even after a restart. Submitting resumes the graph with Command({ resume: answers }). Restrict it to the top-level agent, the same way the manage_notes interrupts are scoped. A subagent is an isolated worker and shouldn't stop to ask the user.
For reference: Claude Code's AskUserQuestion is exactly this shape. It's one always-available tool with 1–4 questions, 2–4 options each, and a multi-select flag plus a free-text "Other" option. The guidance about when to use it lives in the tool description and prompt.
Bugs (these go away with the rework, but for completeness)
askQuestion.ts: the store is keyed on an id LangChain never sets. LangChain passes the model's tool-call id asconfig.toolCall.id(seemanageNotes.ts), notconfig.toolCallId. So in real runs the entry is keyed byconfig.runId, which never matches thetool.idthe card looks up. The unit test passes only because it injectstoolCallIdinto the config by hand.pendingQuestionStore.svelte.tsfindEntry: the fallback hides that mismatch and breaks with more than one pending question. Today the card only works because of the "if exactly one entry, return it" fallback. With two pending questions at once, every lookup misses, Submit drops the answers, and the agent hangs until the user presses Stop. That happens with parallelask_questioncalls, two subagents asking, or two threads each waiting. The card still shows as submitted.AskQuestionCard.svelte: duplicate question ids crash the card. Question ids come from the model and aren't de-duplicated, but they're used as the key of the{#each}and of the selection maps. Two questions with the sameidthroweach_key_duplicate, so the card never renders and the tool waits forever. Assign ids yourself (or de-duplicate them) in the tool.- The tool is bound to subagents through
buildToolsForAgent, so a nestedtaskworker can stop and wait on the UI. clearForThread/getPendingForThreadare never called.
Housekeeping
Please fill in .github/PULL_REQUEST_TEMPLATE.md in the PR body, including the _AI assistance: ..._ line (see AGENTS.md → Contributing).
Happy to help with the interrupt() wiring if useful.
Review drafted with Claude Code from my read-through and a live test in a test vault; I've reviewed and edited it.
Gives the agent the ability to ask interactive multiple-choice questions for clarification.