fix(engine,cli): resolve human gates from dashboard in --web-bg - #324
Merged
Conversation
Extend the #198/#202 max-iterations gate policy to human gates. _handle_gate_with_web previously raced the CLI prompt against the web dashboard unconditionally; in a --web-bg child (stdin=DEVNULL), Prompt.ask raised EOFError instantly, race-won, cancelled the web arm, and crashed the workflow before a dashboard user could respond. - engine/workflow.py: add a cli_usable = not self._bg_mode and sys.stdin.isatty() tier to _handle_gate_with_web. When false, wait web-only via _wait_for_web_gate (no CLI arm). Foreground TTY keeps the existing CLI/web race. Kill/stop while parked at a gate is handled by the existing outer _execute_with_stop_signal path (checkpoints via handle_dashboard_stop, issue #245) -- no new inner stop race needed. - cli/app.py: replace the pre-fork _abort_web_bg_if_human_gate abort with _workflow_has_human_gate detection + a post-launch _print_web_bg_human_gate_notice pointing at the dashboard URL and `conductor gate-respond`. Applies to both run --web-bg and resume --web-bg (including resume --from <checkpoint> without a workflow arg). - docs/cli-reference.md: update the --web-bg + human_gate section to describe dashboard/CLI gate resolution instead of the incompatibility. - plugins/conductor/skills/conductor/references/execution.md: document gate resolution via the dashboard/gate-respond for --web and --web-bg. - tests: rewrite the "aborts before fork" CLI tests to assert the fork proceeds with a gate notice (run, resume, resume-from-checkpoint-only, human_gate nested in for_each, --skip-gates suppresses the notice, gate-free workflows show no notice). Add engine tests asserting the bg-mode gate waits web-only (CLI handler never invoked) and that foreground TTY still races CLI vs web. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address code-review findings on #324: - engine/workflow.py: _handle_gate_with_web now raises HumanGateError when bg_mode is active and no web dashboard is attached (e.g. the dashboard failed to start in a --web-bg child), instead of silently falling through to the CLI prompt handler and crashing with an uncaught, contextless EOFError -- the same failure class issue #286 was filed to fix, reachable via a narrower trigger. The no-dashboard CLI-only path is preserved for non-bg-mode runs (foreground, piped stdin, tests), which were already safe and must not be newly restricted. - tests/test_engine/test_workflow.py: add a regression test asserting bg_mode + no dashboard raises HumanGateError without ever invoking the CLI handler; strengthen the foreground-TTY race test to assert the CLI handler was actually invoked (previously it only asserted the web response won, which passed even if the race never occurred). - cli/app.py: log (debug) when the best-effort human_gate probe fails to load a workflow, instead of swallowing silently; document the urlparse(url).port invariant relied on by the gate notice; fix a grammatically broken comment at the resume() call site. - plugins/conductor/skills/conductor/references/execution.md: soften "fully compatible" overclaim to note the dashboard-failed-to-start case now surfaces a clear error rather than hanging or crashing. 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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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 #286.
Problem
conductor run --web-bg(andresume --web-bg) aborted at launch when the workflow contained ahuman_gate, even though the web dashboard fully supports gate resolution (gate modal, WebSocketgate_response,POST /api/gate-respond,conductor gate-respondCLI).Root cause:
_handle_gate_with_webraced the CLI prompt against the dashboard unconditionally. In the detached--web-bgchild (stdin=DEVNULL),Prompt.askraisedEOFErrorinstantly, race-won, cancelled the web arm, and crashed the workflow before a dashboard user could respond.Precedent: issue #198/#202 fixed the identical problem for the max-iterations gate via a
cli_usabletier in_resolve_max_iterations_gate. The human-gate path never got that tier.Changes
engine/workflow.py:_handle_gate_with_webnow computescli_usable = not self._bg_mode and sys.stdin.isatty(). When false (bg mode or non-TTY), it waits web-only via_wait_for_web_gate— the CLI prompt is never invoked. Foreground TTY keeps the existing CLI/web race. Kill/stop while parked at a gate relies on the existing outer_execute_with_stop_signalpath (checkpoints viahandle_dashboard_stop, Bug: Web Stop/Kill can terminate a run without writing a checkpoint #245) — no new inner stop race was added.cli/app.py: replaced the pre-fork_abort_web_bg_if_human_gateabort with_workflow_has_human_gatedetection + a post-launch_print_web_bg_human_gate_noticepointing at the dashboard URL andconductor gate-respond. Applies to bothrun --web-bgandresume --web-bg(includingresume --from <checkpoint>with no workflow arg).docs/cli-reference.mdand the conductor skill'sexecution.mdto describe dashboard/CLI gate resolution instead of the old incompatibility.human_gatenested infor_each,--skip-gatessuppresses the notice, gate-free workflows show no notice). Added engine tests asserting the bg-mode gate waits web-only (CLI handler never invoked) and that foreground TTY still races CLI vs web.Design decisions (confirmed with maintainer)
wait_for_stop()race inside the gate wait itself, unlike the max-iterations precedent.Testing
make lint/make typecheckclean (one pre-existing, unrelatedtywarning indialog_evaluator.pyconfirmed present onmain).tests/test_cli/,tests/test_engine/,tests/test_gates/,tests/test_web/: 1525 passed, 4 skipped.