Uh oh!
There was an error while loading. Please reload this page.
feat: add a questions node for asking a human a set of questions - #379
Merged
Conversation
prompt_for inputs were single-line on both surfaces: Prompt.ask in the terminal and <input type="text"> in the dashboard, both submitting on Enter. Typing a multi-paragraph answer silently truncated it at the first newline. Add GateOption.multiline, defaulting to False so every existing gate keeps byte-identical behavior. When enabled: - Terminal reads lines until a lone "." or EOF. A sentinel is used rather than adding prompt-toolkit, because _handle_gate_with_web cancels the losing CLI task when the web arm wins and an abandoned asyncio.to_thread never actually dies — a cancelled prompt-toolkit session would strand the terminal in raw mode, where Rich only leaks a blocked thread. - Non-TTY stdin falls back to the single-line path unconditionally. Multi-line editing is meaningless on a pipe, and that path's EOFError-on-closed-stdin behavior is what _handle_gate_with_web is built around. - Dashboard renders a textarea; Enter inserts a newline and Ctrl/Cmd+Enter submits, alongside the Submit button that already existed. Groundwork for the questions node (#376), which defaults its free-text answers to multi-line, but useful on its own. Refs #376 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
_handle_gate_with_web already took an AgentDef and never consulted the workflow graph — it read only name, prompt, and options. The seam existed; it was just expressed in terms of user config rather than an interaction. Introduce GatePrompt / GateChoice / GateResponse and split the layers: - HumanGateHandler.prompt(GatePrompt) -> GateResponse presents one prompt and collects one answer, knowing nothing about the workflow graph. - WorkflowEngine._resolve_human_prompt(GatePrompt) owns the environment policy verbatim — the #286 EOFError fix, the unconditionally-started web task, the non-TTY-without-dashboard arm, the bg-without-dashboard raise, and the #245 killable parked gate. - handle_gate and _handle_gate_with_web survive as thin human_gate adapters that map a response value back to a route. GateChoice deliberately has no route. Routing is a workflow-graph concern, and its absence is what will stop a future caller that merely records an answer from having to invent a sentinel route for every synthesized choice. Behavior-preserving; landed separately so a gate regression bisects to the refactor rather than to the feature that motivates it. Three engine tests patched gate_handler.handle_gate to stand in for the CLI arm and are repointed to .prompt. Two of them assert_not_called() as #286 regression guards and would otherwise have passed vacuously — verified by sabotaging the web-only shortcut and confirming the guard fails. _auto_select is removed rather than left unused; its behavior is covered through the public handle_gate by TestHumanGateHandlerSkipGates, and the two deleted tests exercised the private helper directly. Refs #376 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
human_gate handles one decision well but has no answer to "an agent has N questions and wants a human to work through them". Three workflows in conductor-workflows independently invented the same workaround: a gate that loops back through a set step accumulating a string transcript. That loop has two structural problems. It cannot support going back — a workflow step cannot be un-executed, and a concatenated transcript has no addressable per-question answer to overwrite — and it costs 2 iterations per question against limits.max_iterations. Add `type: questions`: N prompts inside ONE engine step, with the cursor and answers held internally. Answers are a keyed dict, which is what makes revisiting question 3 overwrite answers.q3 rather than append. - name: ask type: questions source: architect.output.open_questions # or inline `questions:` routes: - to: finalize Source entries may be plain strings or objects, so an agent already emitting `array of string` migrates untouched and gaining `choices` later is a backward-compatible upgrade — turning "answer this" into "pick one, or write your own". Details worth calling out: - --skip-gates never selects a suggested answer. Those come from the upstream agent, so recording one would feed invented input back as though a human had provided it. Questions with a `default` take it; the rest are skipped. - Every prompt shares the node's name, so a click meant for Q3 landing after Q4 opened would resolve Q4 with Q3's answer. Each presentation now carries a prompt_id staleness token, matching the pattern already used for iteration-limit gates. A response without one is still accepted so `conductor gate respond` keeps working. - A closing review lists the answers and offers Finish or Back. Without it, answering the last question would end the node instantly, making Back unusable exactly where a user is most likely to want it. Skipped when allow_back is false. - Partial answers are committed to the context after every response, so a mid-node checkpoint already carries them and resume continues at the first unanswered question. No checkpoint schema change — checkpoints serialize WorkflowContext.to_dict(). Answers to questions that no longer exist are dropped rather than resurrected. - `required` blocks submission, never navigation, so a user is never trapped. - The nav flags are tri-state (bool | None) so the schema can reject them on other step types; a plain bool default would make "set to False" and "not set" indistinguishable. - _workflow_has_human_gate now covers questions, or a questions-only workflow launched with --web-bg would park silently with no notice explaining why. Closes#376 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Multi-agent review of #379 surfaced two silent-correctness bugs, several unenforced invariants, and a thread leak the questions node amplifies. Correctness: - Re-entering the node via an ordinary route loop-back replayed the previous pass's answers. Ids default to positional q1..qN, so a new question set silently inherited old answers; with allow_back: false the node presented ZERO prompts and reported "completed". Restore is now consumed once, set only by resume(). - Restore matched on id alone, so any edit to a question's text or order misfiled the old answer onto a different question. Text must now match too, and every drop is logged — a silently unanswered question is asked again, a silently misattached one never is. - Question text from source: was Jinja-rendered, so an ordinary model-authored question ("Should this use {{ user.id }}?") aborted the step with a TemplateError just as the human was about to be asked. Source text is now verbatim, matching for_each; inline questions keep full rendering. - required was bypassable in one click via skip-all, which is evaluated before the per-question check. - Back at question 1 (reachable via a duplicate click) popped an answer without moving the cursor. - Mid-node progress commits went through context.store, adding N entries to execution_history and current_iteration — visible in downstream prompts, the interrupt panel, and the dashboard's synthetic replay. - conductor gate respond sent additional_input as a bare string, which the engine discarded: the operator's typed answer vanished while the CLI printed success. It also never echoed prompt_id, so an answer could land on whichever question opened between its status GET and its POST. Validation (all were accepted, then failed at runtime in front of a human): - abort_route was unvalidated and absent from the routing graph. - source had no format check despite claiming parity with ForEachDef.source. - questions was documented as rejected in for_each groups but ran as an LLM agent instead. - A question with no choices and allow_free_text: false was unanswerable. - A suggested answer colliding with a control sentinel is now refused. Silent failures: - questions_answer_rejected rendered nowhere in the terminal, and the dashboard cleared it on the gate_presented that always follows. The reason now rides inside the re-presented prompt, and the console subscriber renders it. - An empty source: array emitted no events, leaving the dashboard node pending forever. - An unknown source in a restored checkpoint is rejected rather than passed through to the dashboard. Thread leak: cancelling the losing CLI arm does not stop its asyncio.to_thread worker, which keeps a slot in the shared default executor. A questions node abandons one per question, so the executor drains, every unrelated to_thread in the process blocks forever, and loop.shutdown_default_executor() hangs at exit. Stdin reads now run on dedicated daemon threads, following the same reasoning as interrupt/listener.py. Types: AnswerSource and QuestionsOutcome are Literal aliases (matching CheckpointTrigger), and AnswerRecord.skipped is derived from source rather than stored, so the contradictory state reachable through checkpoint restore no longer exists. Tests: two tests passed vacuously (verified by mutation) and are rewritten; adds coverage for the prompt_id staleness guard, the frontend store handlers, and the questions schema/validator branches — all previously untested. Refs #376 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The node output crosses a Jinja/JSON boundary as a plain dict, so the outcome is compared as a bare string literal. A mistyped literal would silently stop matching and disable the abort route; a mistyped name is an immediate NameError. Verified by mutation: changing the constant fails both abort tests, so the heavier typed-output-dataclass refactor the review suggested is not needed to close this risk. Refs #376 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Array.push returns a number, so the stub was not assignable to (data: object) => void. A cached tsc -b masked this locally; CI typechecks from clean. Refs #376 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolves conflicts in two generated artifacts by rebuilding from the merged source rather than picking a side: - src/conductor/web/static/** (hashed bundle + index.html) - src/conductor/web/frontend/tsconfig.tsbuildinfo All hand-written source auto-merged. #377 added real frontend source (graph-anchor, camera-authority, use-deep-link) whose 47 tests pass alongside this branch's 79. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes#376.
human_gatehandles one decision well. It has no answer to "an agent has N questions and wants a human to work through them" — and three workflows inmicrosoft/conductor-workflowsindependently invented the same workaround.sdd/plan.yamlask_question→answer_logloopsdd/design.yamldocument/review.yamlprompt_forblobThat loop has two structural problems, not just missing features. It cannot support going back — a workflow step cannot be un-executed, and a concatenated transcript string has no addressable per-question answer to overwrite — and it costs 2 iterations per question against
limits.max_iterations.Three commits
1.
feat(gates): opt-in multi-line text input— stands aloneprompt_forwas single-line on both surfaces (Prompt.ask,<input type="text">), so a multi-paragraph answer was silently truncated at the first newline. AddsGateOption.multiline, defaulting toFalseso every existing gate is byte-identical.A sentinel reader (
.on its own line, or EOF) rather than aprompt-toolkitdependency:_handle_gate_with_webcancels the losing CLI task when the web arm wins, and an abandonedasyncio.to_threadnever actually dies — a cancelled prompt-toolkit session would strand the terminal in raw mode, where Rich only leaks a blocked thread. Non-TTY stdin falls back to the single-line path unconditionally, keeping theEOFErrorbehaviour_handle_gate_with_webis built around.2.
refactor(gates): extract GatePrompt— behaviour-preserving_handle_gate_with_webalready took anAgentDefand never consulted the workflow graph — it read onlyname,prompt,options. The seam existed; it was just expressed in terms of user config. IntroducesGatePrompt/GateChoice/GateResponseand moves the environment policy (the #286EOFErrorfix, the unconditionally-started web task, the non-TTY arm, the bg-without-dashboard raise, the #245 killable parked gate) into a graph-agnostic_resolve_human_prompt.GateChoicedeliberately has noroute. Routing is a graph concern, and its absence is what stopsquestionsinventing a sentinel route for every synthesized choice.Landed separately so a gate regression bisects to the refactor rather than to the feature.
3.
feat(engine): add a questions nodeN prompts inside one engine step. Answers are a keyed dict, which is what makes revisiting question 3 overwrite
answers.q3rather than append.Source entries may be plain strings or objects, so an agent already emitting
array of stringmigrates untouched, and addingchoiceslater is a backward-compatible upgrade that turns "answer this" into "pick one, or write your own".Details worth reviewing
--skip-gatesnever selects a suggested answer. Those come from the upstream agent, so auto-selectingoptions[0]would feed invented input back as though a human had provided it — worse than the current loop, which skips honestly. Questions with adefaulttake it; the rest are skipped.prompt_idtoken, matching the pattern already proven for iteration-limit gates. A response without one is still accepted, soconductor gate respondkeeps working.allow_back: false, where it would be pure friction.WorkflowContext.to_dict()— so no checkpoint schema change. Resume continues at the first unanswered question; answers to questions that no longer exist are dropped rather than resurrected.bool | None) so the schema can reject them on other step types. A plainbooldefault makes "set toFalse" and "not set" indistinguishable. (I first triedmodel_fields_set— the existing round-trip tests correctly caught thatmodel_dump()emits defaults.)_workflow_has_human_gatenow coversquestions, or aquestions-only workflow launched with--web-bgwould park silently with no notice explaining why.Verification
-m "not performance");make checkandmake test-frontendclean;make validate-examplespasses.gate_handler.handle_gateas a stand-in for the CLI arm, repointed to.prompt. Two of them areassert_not_called()--web-bgblocks workflows withhuman_gateeven though the dashboard fully supports gate resolution — extend the #198 policy to human gates #286 regression guards that would otherwise have passed vacuously — verified by sabotaging the web-only shortcut and confirming the guard fails.tests/test_performance.py::TestForEachPerformance::test_batching_scalabilityfails on this branch and identically on unmodifiedorigin/main— pre-existing, unrelated.Follow-up (separate repo)
Migrate
sdd/plan.yamlandsdd/design.yaml— deletes ~90 lines of duplicated Jinja and bothanswer_lognodes — and consider promotingdocument/review.yamlfrom the single-blob gate to real per-question answers.Review round
Seven specialist agents reviewed this PR. They found two silent-correctness bugs I'd missed, several unenforced invariants, and a thread leak the node amplifies — all fixed in
c63bd04.Correctness
q1..qN, so a second pass over a different question set silently inherited the first pass's answers; withallow_back: falseit presented zero prompts and reportedcompleted. Restore is now consumed once and set only byresume().source:text was Jinja-rendered —Should this use {{ user.id }}?is an ordinary question for a developer tool, and it aborted the step with aTemplateErrorexactly when the human was about to be asked. Source text is now verbatim (matchingfor_each); inline questions keep full rendering.requiredwas bypassable in one click via skip-all, which runs before the per-question check.conductor gate respond --inputsilently discarded the answer (sent as a bare string, coerced to{}) while printing success, and never echoedprompt_id, so an answer could land on whichever question opened between its status GET and its POST.execution_history.Validation —
abort_route,sourceformat,questionsin for-each groups, and unanswerable questions were all accepted and then failed at runtime in front of a human.Thread leak — cancelling the losing CLI arm doesn't stop its
asyncio.to_threadworker, which keeps a slot in the shared default executor. One leaks per question, so the executor drains, every unrelatedto_threadin the process blocks forever, andloop.shutdown_default_executor()hangs at exit. Verified: 12 abandoned reads previously hungasyncio.run(); they are now daemon threads, the default executor stays usable, and the interpreter exits cleanly.Tests — two tests passed vacuously (proven by mutating production code and watching them still pass) and were rewritten. Added coverage for the
prompt_idstaleness guard, the frontend store handlers, and the schema/validator branches — all previously untested. The frontend gap had already hidden a real bug: Back emits a secondquestions_answered, so the monotonic counter reached "3 of 2" and rendered a 150%-wide progress bar.Final: 4967 passed, 37 skipped (
-m "not performance"); 79 frontend tests;make checkandmake validate-examplesclean.