diff --git a/.gitignore b/.gitignore index fbdbbf9b..c951c66c 100644 --- a/.gitignore +++ b/.gitignore @@ -83,3 +83,6 @@ src/conductor/web/frontend/node_modules/ src/conductor/designer/frontend/node_modules/ .playwright-mcp/ + +# Scratch / temporary working files +tmp/ diff --git a/CHANGELOG.md b/CHANGELOG.md index ab525d1f..02f79946 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -278,6 +278,55 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#225](https://github.com/microsoft/conductor/pull/225), [#136](https://github.com/microsoft/conductor/issues/136)). +### Added +- New `output_mode` field on `AgentDef` (`raw` | `envelope`). Setting + `output_mode: raw` bypasses JSON schema injection and parse-recovery entirely, + wrapping the model's response as `{"result": ""}`. Useful for agents + that produce large Markdown, prose, or code output that should not be + JSON-extracted. `output_mode: raw` is incompatible with `output:` — declaring + both raises a `ValidationError` at config load time. +- New `max_parse_recovery_attempts` field on `RetryPolicy` (YAML `retry:` + block, per-agent or workflow-level). Overrides the provider default (Copilot: + 5, Claude: 2) for agents that need tighter or looser in-session parse-recovery + budgets. Accepts integer 0–10; `0` disables all recovery attempts and lets + the first parse failure propagate immediately. Threaded through both the + Copilot and Claude providers. +- New `POST /api/gate-respond` and `GET /api/gate-status` HTTP API endpoints + on the web dashboard server. `GET /api/gate-status` returns whether a + `human_gate` agent is currently waiting, and which agent name it is. + `POST /api/gate-respond` resolves the parked gate by injecting a + `GateResponse` into the engine's queue. When the optional + `CONDUCTOR_GATE_TOKEN` secret is configured on the server, `POST + /api/gate-respond` requires an `Authorization: Bearer ` header + matching it (compared in constant time) — requests with a missing or + mismatched token are rejected with HTTP 403. `GET /api/gate-status` is + unauthenticated. The matching WebSocket `gate_response` path enforces the + same token and waiting-state checks so it cannot be used to bypass auth. +- New `conductor gate-respond` CLI command for resolving a parked human gate + from the command line without opening a browser. Accepts `--port`, `--choice`, + `--agent` (auto-discovered via `/api/gate-status` when omitted), `--input`, + and `--token` / `CONDUCTOR_GATE_TOKEN` env var. Designed for SSH or headless + environments where the web dashboard UI is unreachable. +- `script` steps now resolve a bare command name (e.g. `python`) or an + extension-less path against the executable search path before launching, so + the binary the shell would pick is the one that runs (and a Windows path + missing its `.exe`/`.cmd` suffix resolves correctly). Resolution uses the + subprocess's own `PATH` — including any `env.PATH` override on the step — so + the resolved binary matches what the child process would execute. Relative + paths containing a separator are left untouched so they keep resolving against + `working_dir`, and an unresolvable command falls back to the rendered value so + the existing not-found error still fires. + +### Changed +- **Breaking (Claude provider):** `ClaudeProvider._extract_text_content` now + returns `{"result": ""}` instead of `{"text": ""}`. This aligns + the Claude provider with the Copilot provider (cross-provider parity). Any + existing Claude workflow that references `{{ .output.text }}` must be + updated to `{{ .output.result }}`. Workflows that declare an `output:` + schema are unaffected (the schema fields take precedence). See the new + `output_mode: raw` feature if you need to consume unstructured text output + reliably across both providers. + ### Fixed - `_verbose_console` is now silent-aware at the source: a `_SilentAwareConsole` subclass no-ops every `.print(...)` when `is_verbose()` is False, so the @@ -291,6 +340,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 bytes on stderr ([#223](https://github.com/microsoft/conductor/pull/223), closes [#209](https://github.com/microsoft/conductor/issues/209)). +- Parse-exhaustion `ProviderError` (after all in-session recovery attempts + are spent) is now marked `is_retryable=False` in both Copilot and Claude + providers. Previously Copilot marked it `is_retryable=True`, causing the + outer retry loop to re-run the entire agent up to 3× on deterministic + parse failures — burning tokens with no chance of success. +- Parse-exhaustion error messages now include the first 500 characters of the + model's response (up from 200) and suggest `output_mode: raw` as a fix. - `parse_json_output` and the Copilot provider's `_extract_json` now use a two-stage fenced-block extraction (non-greedy `re.findall` + per-candidate try-parse, then a greedy single-capture fallback) so JSON whose string diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 19653615..dada2cc4 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -6,6 +6,7 @@ Complete command-line reference for Conductor. - [`conductor run`](#conductor-run) - [`conductor stop`](#conductor-stop) +- [`conductor gate-respond`](#conductor-gate-respond) - [`conductor validate`](#conductor-validate) - [`conductor registry`](#conductor-registry) @@ -107,7 +108,7 @@ load time and aborts before forking, with a message listing the options: 1. Use `--web` (foreground) instead of `--web-bg` 2. Add `--skip-gates` to auto-select the first option at every gate 3. Remove `human_gate` steps from the workflow -4. Wait for CLI gate-resolution support (planned follow-up) +4. Use `conductor gate-respond --port --choice ` to resolve from CLI The same check applies to `conductor resume --web-bg`. @@ -234,6 +235,58 @@ conductor stop --port 8080 conductor stop --all ``` +## `conductor gate-respond` + +Resolve a parked `human_gate` step from the command line without opening a browser. Sends a gate response to a running workflow's web dashboard via HTTP — useful for SSH sessions or headless environments where the dashboard UI is unreachable. + +```bash +conductor gate-respond [OPTIONS] +``` + +### Options + +| Option | Short | Description | +|--------|-------|-------------| +| `--port PORT` | `-p` | Dashboard port of the running workflow (**required**) | +| `--choice VALUE` | `-c` | Selected gate option value (**required**) | +| `--agent NAME` | `-a` | Gate agent name (auto-discovered via `/api/gate-status` when omitted) | +| `--input TEXT` | | Additional free-text input for the gate response | +| `--token SECRET` | | Auth token (also reads from `CONDUCTOR_GATE_TOKEN` env var) | + +### Authentication + +If the running workflow was launched with a gate token configured, requests without a matching token are rejected with HTTP 403. Supply the token via `--token` or set the `CONDUCTOR_GATE_TOKEN` environment variable (the flag takes precedence when both are present). + +### Auto-Discovery + +When `--agent` is omitted, `conductor gate-respond` queries `GET /api/gate-status` on the specified port. If a gate is currently waiting, its agent name is used automatically and printed to the console. If no gate is waiting, the command exits with code 1. + +### Examples + +```bash +# Resolve the only waiting gate (agent auto-discovered) +conductor gate-respond --port 8080 --choice approve + +# Resolve a specific named gate +conductor gate-respond -p 8080 -c reject --agent review-gate + +# Pass additional free-text input +conductor gate-respond -p 8080 -c approve --input "Looks good, ship it" + +# Provide auth token via flag +conductor gate-respond -p 8080 -c approve --token my-secret + +# Provide auth token via environment variable +CONDUCTOR_GATE_TOKEN=my-secret conductor gate-respond -p 8080 -c approve +``` + +### Exit Codes + +| Code | Meaning | +|------|---------| +| 0 | Gate resolved successfully | +| 1 | Connection error, auth failure, validation error, or no gate waiting | + ## `conductor validate` Validate a workflow file without executing it. Checks YAML syntax, schema compliance, cross-references (agent names, routes, parallel groups), and Jinja2 template references throughout the workflow. @@ -364,6 +417,7 @@ See [design/registry.md](./design/registry.md) for the full design. | `ANTHROPIC_API_KEY` | API key for Claude provider | | `GITHUB_TOKEN` | Token for Copilot provider (if not using GitHub CLI auth) | | `CONDUCTOR_LOG_LEVEL` | Logging level: DEBUG, INFO, WARNING, ERROR | +| `CONDUCTOR_GATE_TOKEN` | Auth token required by `conductor gate-respond` (and checked by `POST /api/gate-respond`) when the workflow dashboard is started with a gate token | ## Exit Codes diff --git a/docs/projects/usability-features/external-workflow-friction-v2.plan.md b/docs/projects/usability-features/external-workflow-friction-v2.plan.md new file mode 100644 index 00000000..576b7660 --- /dev/null +++ b/docs/projects/usability-features/external-workflow-friction-v2.plan.md @@ -0,0 +1,620 @@ +# Solution Design: External Workflow Friction v2 — Remaining Gaps + +**Status:** DONE (EPICs 1–4 SHIPPED) +**Revision:** 3 — Rebased onto actual v0.1.17 codebase state (score 68 review) +**Prior plan:** [external-workflow-friction.plan.md](external-workflow-friction.plan.md) (SHIPPED — all four items landed in v0.1.17) +**Source brainstorm:** [external-workflow-friction.brainstorm.md](external-workflow-friction.brainstorm.md) (updated 2026-05-27 with Phase 1/2/3 validation evidence) +**Author:** Lucio Tinoco (external contributor) + Copilot design +**Conductor version:** v0.1.17 + +--- + +## 1. Executive Summary + +The prior plan shipped four fixes in v0.1.17: greedy fence regex (#1), script-in-parallel rejection (#7), `--web-bg` + `human_gate` guard (#8), and documentation for the "omit `output:`" pattern (#2 docs-only). A three-phase validation pass against v0.1.17 identified five remaining gaps. **Two of those five have since been fully implemented:** + +- **`output_mode: raw | envelope`** (Issue #2) — SHIPPED. The `output_mode` field is implemented in `schema.py:508-523` with full validation rules, provider support (`copilot.py:524`, `claude.py:922`), documentation (`docs/workflow-syntax.md:159-203`), and tests. +- **Parse-exhaustion `is_retryable=False`** (Issue #10) — SHIPPED. Both providers mark parse-exhaustion as non-retryable (`copilot.py:748`, `claude.py:1887`), preventing the 3× outer-retry amplification. Error messages include 500-char response prefixes and suggest `output_mode: raw`. + +**Three items constituted this plan's active scope — all now SHIPPED (EPICs 2–4 on branch `epics-1-4`, PR #234):** + +1. **YAML-exposed `max_parse_recovery_attempts`** (Issue #6) — the internal `_retry_config` field from v0.1.17 is now user-configurable from the YAML `retry:` block, and the resolved per-agent value reaches the parse recovery loop in both providers. +2. **CLI `conductor gate-respond`** (Issue #5) — adds a CLI fallback (and `POST /api/gate-respond` endpoint) for resolving a parked gate when the dashboard is unreachable. Hardened during adversarial review with `agent_name` mismatch rejection (`409`) and header-based `hmac` token auth. +3. **Subprocess command resolution** (Issue #3) — resolves `command:` against PATH/PATHEXT (`shutil.which`) in `executor/script.py`. + +The plan was sequenced: EPIC 2 (parse recovery config) first, then EPIC 3 (gate-respond), then EPIC 4 (command resolution). EPIC 1 is retained as a reference section with all tasks marked DONE. + +--- + +## 2. Background + +### Current State + +Conductor v0.1.17 orchestrates multi-agent workflows defined in YAML. When an agent declares an `output:` schema, the provider injects a "respond with JSON matching this schema" instruction and attempts to parse the model's response through: + +1. **Direct `json.loads`** → code-block extraction → brace-pattern extraction (copilot: `_extract_json` at `copilot.py:1081-1122`; claude: `_extract_json_fallback` + `emit_output` tool) +2. **Parse recovery loop** — up to 5 attempts (copilot, `copilot.py:680-748`) or 2 attempts (claude, `claude.py:1805-1888`) sending a correction prompt in the same session +3. **Outer retry loop** — up to 3 attempts (both providers, `copilot.py:75` / `claude.py:100`) restarting the entire agent session. Parse-exhaustion errors are now marked `is_retryable=False` (copilot: line 748, claude: line 1887), so the outer loop does **not** amplify parse failures. + +Agents can now declare `output_mode: raw` (`schema.py:508-523`) to bypass JSON extraction entirely, receiving their response as `{"result": ""}`. Both providers check `has_schema = agent.output and agent.output_mode != "raw"` (copilot: line 524, claude: line 922) before injecting schema instructions. This resolves the ~80 KB response parse failure root cause. + +**What remains unsolved** is that `max_parse_recovery_attempts` (the inner loop limit) is not configurable from YAML. Both providers store it in an internal `_retry_config` (`copilot.py:81`, `claude.py:106`) or instance variable (`claude.py:191`), but `_resolve_retry_config()` always copies from the provider-level default, never from the YAML `RetryPolicy`. + +### What Changed + +The prior plan (v1) deliberately deferred `output_mode` as a non-goal. Phase 3 validation empirically disproved this reasoning. The `output_mode` field was then implemented (along with `is_retryable=False` and 500-char error prefixes) in the period between v0.1.17 and the current HEAD. This revision acknowledges those implementations as DONE and focuses the remaining plan on the three genuinely open items. + +**Provider retry classification parity note:** Copilot's outer retry loop catches `ProviderError` and checks `e.is_retryable` (line 388). Claude's outer retry loop catches `Exception` broadly (line 1026) and uses `self._is_retryable_error(e)` which does `isinstance()` checks against Anthropic SDK exception types (line 713-769). A `ProviderError` raised from parse exhaustion doesn't match any SDK exception type, so `_is_retryable_error()` returns `False`. Both providers achieve the same behavioral outcome (no outer retry on parse exhaustion) through different mechanisms. The explicit `is_retryable=False` on Claude's parse-exhaustion `ProviderError` (line 1887) is correct for documentation clarity but is not the mechanism that prevents outer retry in Claude — `_is_retryable_error()` would return `False` regardless. This asymmetry is a known architectural difference with no behavioral impact for parse-exhaustion specifically. + +--- + +## 3. Problem Statement + +Five issues were identified after v0.1.17. The first two were resolved before this plan's active scope; the remaining three were addressed by EPICs 2–4 (PR #234): + +1. ~~**No `output_mode` field**~~ → **RESOLVED.** `output_mode: raw | envelope` is implemented at `schema.py:508-523` with provider support, validation, tests, and documentation. + +2. ~~**Hidden 3× outer retry on parse exhaustion**~~ → **RESOLVED.** Both providers now raise parse-exhaustion with `is_retryable=False` (`copilot.py:748`, `claude.py:1887`). Error messages include 500-char prefixes and suggest `output_mode: raw`. + +The remaining three (now SHIPPED) were: + +3. ~~**`max_parse_recovery_attempts` not YAML-configurable**~~ → **RESOLVED (EPIC 2).** Both providers respected an internal `_retry_config.max_parse_recovery_attempts` value, but `_resolve_retry_config()` always copied from the provider-level default (`copilot.py:303`, `claude.py:685`), never from the YAML `RetryPolicy`, and the resolved per-agent config did not reach the parse recovery loop. EPIC 2 threads the YAML value through to the recovery loop in both providers. + +4. ~~**No CLI gate-resolution path**~~ → **RESOLVED (EPIC 3).** Gate responses previously flowed exclusively through WebSocket (`web/server.py:326-327`); when the dashboard was unreachable there was no fallback. EPIC 3 adds the `conductor gate-respond` CLI command and `POST /api/gate-respond` endpoint (hardened post-review with `agent_name` 409 validation and header-based `hmac` token auth). + +5. ~~**Subprocess command not resolved against PATH**~~ → **RESOLVED (EPIC 4).** `script.py` passed `rendered_command` to `create_subprocess_exec` without resolving bare names against PATH/PATHEXT. EPIC 4 resolves absolute paths and bare names via `shutil.which`. (Note: the originally-reported intermittent Windows forward-slash `FileNotFoundError` could not be reproduced — forward-slash and extension-less commands execute fine via `create_subprocess_exec`.) + +--- + +## 4. Goals and Non-Goals + +### Goals + +| ID | Goal | Status | +|----|------|--------| +| G1 | Agents producing large/prose output can declare `output_mode: raw` to bypass JSON envelope extraction, receiving their response as `{result: }`. | ✅ DONE | +| G2 | Parse-exhaustion failures are marked `is_retryable=False` by default across both providers (deterministic failures should not retry). Users can opt into outer retries for parse failures via `retry.max_attempts`. | ✅ DONE | +| G3 | `max_parse_recovery_attempts` is configurable from YAML via the `retry:` block on `AgentDef`, with provider-specific defaults preserved for backward compat (Copilot=5, Claude=2). | ✅ DONE | +| G4 | A `conductor gate-respond` CLI command resolves a parked gate by POSTing to the dashboard's HTTP API, with optional token-based auth for shared infrastructure. | DONE | +| G5 | Bare command names and absolute paths in `command:` are resolved against PATH/PATHEXT via `shutil.which` before `create_subprocess_exec` (non-destructive: unresolved commands fall through unchanged; relative paths with a separator are left for `working_dir` resolution), and the `FileNotFoundError` message includes the resolved command and a Windows hint. | DONE | + +### Non-Goals + +| ID | Non-Goal | Rationale | +|----|----------|-----------| +| NG1 | Brace-balanced JSON extractor | Diminishing returns once `output_mode: raw` exists. Consider as a follow-up only if residual parse failures persist. | +| NG2 | Validate-time heuristic warning for `output:` on prose-likely agents | Fragile heuristic (would need NLP on prompt content). Documentation + `output_mode` field suffice. | +| NG3 | Default-flip to `output_mode: raw` in v0.2.x | Too disruptive. Current plan is additive-only. Revisit in a future major version. | +| NG4 | WebSocket keepalive / heartbeat | Orthogonal to the gate-resolution gap. The CLI command is the fallback that makes keepalive optional. | +| NG5 | Webhook notification on gate-waiting | Out of scope for v0.1.x. | + +--- + +## 5. Requirements + +### Functional Requirements + +| ID | Requirement | Status | +|----|-------------|--------| +| FR-1 | `AgentDef` in `config/schema.py` accepts `output_mode: raw \| envelope` (optional, default `None` = current behavior). | ✅ DONE (`schema.py:508-523`) | +| FR-2 | When `output_mode: raw` is set, the provider skips schema instruction injection and JSON envelope extraction, wrapping the response in `{"result": }`. | ✅ DONE (`copilot.py:524,670-678`, `claude.py:922`) | +| FR-3 | `output_mode: envelope` with `output:` declared behaves identically to current behavior (full backward compat). | ✅ DONE | +| FR-4 | `output_mode: raw` with `output:` declared raises `ValidationError` at config load time ("output_mode 'raw' is incompatible with output schema declaration"). | ✅ DONE (`schema.py:798-802`) | +| FR-5 | Parse-exhaustion `ProviderError` in both `copilot.py` and `claude.py` is raised with `is_retryable=False`. | ✅ DONE (`copilot.py:748`, `claude.py:1887`) | +| FR-6 | `RetryPolicy` in `config/schema.py` accepts `max_parse_recovery_attempts: int` (optional, default `None` = provider default). | ✅ DONE | +| FR-7 | `_resolve_retry_config()` in both providers propagates the per-agent `max_parse_recovery_attempts` when set. | ✅ DONE | +| FR-8 | Parse-failure error messages include the first 500 characters of the response. | ✅ DONE (`copilot.py:744`, `copilot.py:1122`) | +| FR-9 | `conductor gate-respond` CLI command accepts `--port` (to identify the running instance) and `--choice` / `--input` to resolve the gate. | DONE | +| FR-10 | `web/server.py` exposes a `POST /api/gate-respond` HTTP endpoint accepting JSON `{agent_name, selected_value, additional_input?}`. The endpoint returns `409` when no gate is currently waiting or when `agent_name` does not match the waiting gate (post-review: prevents a mismatched response being silently queued so the gate never resolves). | DONE | +| FR-11 | Gate-respond endpoint validates an optional `CONDUCTOR_GATE_TOKEN` env var when set. The token is read from the `Authorization: Bearer ` header (post-review: moved off the JSON body) and compared with `hmac.compare_digest` for constant-time matching. | DONE | +| FR-12 | `script.py` resolves `rendered_command` via `shutil.which` when it is an absolute path or a bare name (no separator) before calling `create_subprocess_exec`, falling back to the rendered value when unresolved. Args are never resolved (they may contain URLs or flags with `/`). | +| FR-13 | The `FileNotFoundError` handler in `script.py` includes the resolved command, working directory, and (on Windows) a hint to include the file extension or use an absolute path. | + +### Non-Functional Requirements + +| ID | Requirement | +|----|-------------| +| NFR-1 | Every FR has at least one unit/integration test that fails before the fix and passes after. | +| NFR-2 | `make check` (lint + typecheck) and `make test` pass after each epic. | +| NFR-3 | Provider parity: any behavioral change in `copilot.py` is mirrored in `claude.py`. | +| NFR-4 | Run/resume parity: any new CLI flag on `run` is mirrored on `resume` per AGENTS.md. | + +--- + +## 6. Proposed Design + +### 6.1 Architecture Overview + +``` +┌─────────────────────────────────────────────────────────────┐ +│ config/schema.py │ +│ AgentDef │ +│ ├── output_mode: Literal["raw","envelope"] | None ✅ DONE │ +│ ├── output: dict[str, OutputField] | None │ +│ └── retry: RetryPolicy | None │ +│ └── max_parse_recovery_attempts: int | None (NEW) │ +├─────────────────────────────────────────────────────────────┤ +│ providers/copilot.py + claude.py │ +│ _execute_sdk_call / _execute_with_retry │ +│ ├── Check agent.output_mode → skip schema if raw ✅ DONE │ +│ ├── Parse-exhaustion → is_retryable=False ✅ DONE │ +│ └── _resolve_retry_config → propagate per-agent │ +│ max_parse_recovery_attempts (FIX) │ +├─────────────────────────────────────────────────────────────┤ +│ executor/script.py │ +│ ├── Resolve command via shutil.which (PATH/PATHEXT) (NEW) │ +│ └── Improved FileNotFoundError message (FIX) │ +├─────────────────────────────────────────────────────────────┤ +│ web/server.py │ +│ └── POST /api/gate-respond endpoint (NEW) │ +├─────────────────────────────────────────────────────────────┤ +│ cli/app.py │ +│ └── conductor gate-respond command (NEW) │ +└─────────────────────────────────────────────────────────────┘ +``` + +### 6.2 Key Components + +#### 6.2.1 `output_mode` Field on `AgentDef` (Issue #2) — ✅ SHIPPED + +**Status:** Fully implemented and tested. Retained here as a reference for the design rationale. + +**Location:** `src/conductor/config/schema.py`, `AgentDef` class (line 508) + +The `output_mode` field is defined at `schema.py:508-523`: + +```python +output_mode: Literal["raw", "envelope"] | None = None +``` + +**Validation rules (model_validator on AgentDef, line 700):** +- `output_mode: raw` + `output:` declared → `ValidationError` (line 798-802) +- `output_mode` on `human_gate` → `ValidationError` (line 717-718) +- `output_mode` on `script` → `ValidationError` (line 753-754) +- `output_mode` on `workflow` → `ValidationError` (line 782-783) + +**Provider behavior (implemented):** + +| `output_mode` | `output:` | Schema injected? | JSON extracted? | Content shape | +|---|---|---|---|---| +| `None` | present | Yes | Yes (current) | `{field1: ..., field2: ...}` | +| `None` | absent | No | No (current) | `{"result": ""}` | +| `raw` | absent | No | No | `{"result": ""}` | +| `envelope` | present | Yes | Yes | `{field1: ..., field2: ...}` | +| `envelope` | absent | No | No | `{"result": ""}` | +| `raw` | present | ❌ ValidationError | — | — | + +**Implemented in providers:** + +- **Copilot** (`copilot.py:524`): `has_schema = agent.output and agent.output_mode != "raw"` — skips schema instruction injection when `output_mode == "raw"`. +- **Copilot** (`copilot.py:670-678`): When `not has_schema`, wraps as `{"result": response_content}`. +- **Claude** (`claude.py:922`): `has_schema = agent.output is not None and agent.output_mode != "raw"` — skips `emit_output` tool injection. + +**Tests:** `tests/test_config/test_output_mode.py` (10 schema tests), `tests/test_providers/test_output_mode.py` (10+ provider behavior tests). + +**Docs:** `docs/workflow-syntax.md:159-203`, `CHANGELOG.md:11-16`. + +#### 6.2.2 Parse Recovery Config + Outer Retry Budget (Issues #10 + #6) + +**Issue #10 status: ✅ RESOLVED.** Both providers mark parse-exhaustion as `is_retryable=False` (`copilot.py:748`, `claude.py:1887`). The 3× outer-retry amplification no longer occurs. + +**Issue #6 status: ✅ RESOLVED (EPIC 2, PR #234).** `max_parse_recovery_attempts` is now user-configurable from YAML and threaded through to the parse recovery loop in both providers. The root-cause analysis below is retained as design context. + +**Root cause of Issue #6:** + +In `copilot.py`, the `RetryConfig` dataclass defaults `max_parse_recovery_attempts=5` (line 81). When the agent has a per-agent `retry:` policy, `_resolve_retry_config()` (line 278-304) builds a new `RetryConfig` but copies `max_parse_recovery_attempts` from `self._retry_config.max_parse_recovery_attempts` (line 303) — always the provider-level default, never from YAML. + +In `claude.py`, the same `RetryConfig` defaults `max_parse_recovery_attempts=2` (line 106). `_resolve_retry_config()` (line 660-686) copies from the provider-level default at line 685. + +**Critical threading gap (resolved by EPIC 2):** + +Even if `_resolve_retry_config()` propagated a per-agent value, it would not reach the parse recovery loop: + +- **Copilot**: `_execute_with_retry` (line 306) resolves the config at line 335, but `_execute_sdk_call` reads `self._retry_config.max_parse_recovery_attempts` at line 681 (the provider-level default, not the per-agent resolved config). Fix: pass the resolved `RetryConfig` from `_execute_with_retry` into `_execute_sdk_call` as a parameter, and use it at line 681. +- **Claude**: `_execute_with_parse_recovery` (line 1738) reads `self._max_parse_recovery_attempts` (an instance variable set at line 191 from the provider-level default, not the resolved config). Fix: add a `max_parse_recovery_attempts` parameter to `_execute_with_parse_recovery` and pass the resolved value from the agentic loop. + +**Fix — Expose `max_parse_recovery_attempts` in YAML schema:** + +Add to `RetryPolicy` in `config/schema.py` (after line 395): + +```python +max_parse_recovery_attempts: int | None = Field(default=None, ge=0, le=10) +"""Maximum in-session parse-recovery attempts before giving up. + +When an agent's response fails JSON extraction, Conductor sends a +correction prompt in the same session. This field controls how many +correction prompts to send. + +- ``None`` (default): Use the provider default (Copilot=5, Claude=2). +- ``0``: Disable parse recovery entirely (fail immediately on bad JSON). +- ``1-10``: Custom limit. +""" +``` + +Update `_resolve_retry_config()` in both providers (`copilot.py:278-304`, `claude.py:660-686`) to propagate the per-agent value: + +```python +# In _resolve_retry_config, when building RetryConfig from RetryPolicy: +max_parse = retry.max_parse_recovery_attempts # from YAML +if max_parse is None: + max_parse = self._retry_config.max_parse_recovery_attempts # provider default +return RetryConfig( + ... + max_parse_recovery_attempts=max_parse, +) +``` + +Then thread the resolved config to the parse recovery loop (see EPIC 2 tasks for details). + +**Part C — Longer error prefix in parse-failure messages: ✅ SHIPPED** + +Both truncation points already use 500-char prefixes: +- `copilot.py:744`: `response_content[:500]` +- `copilot.py:1122`: `content[:500]` (in `_extract_json`) +- Suggestion text mentions `output_mode: raw` (`copilot.py:745-746`, `claude.py:1884-1885`) + +#### 6.2.3 CLI `gate-respond` Command (Issue #5) — ✅ SHIPPED + +> **Hardening (post-review):** Two refinements were applied during adversarial +> review. (1) The endpoint now rejects responses that don't match the waiting +> gate — if no gate is waiting or `agent_name` differs from the waiting agent it +> returns `409` instead of silently queuing a payload that would never resolve +> the gate (and could poison a later, unrelated gate). (2) The auth token moved +> off the JSON body to the `Authorization: Bearer ` header and is compared +> with `hmac.compare_digest` for constant-time matching. + +**HTTP endpoint** in `web/server.py`: + +```python +@app.post("/api/gate-respond") +async def gate_respond_api(request: Request) -> JSONResponse: + """Resolve a parked human gate via HTTP POST. + + Body: {"agent_name": str, "selected_value": str, "additional_input": str?} + Auth: optional `Authorization: Bearer ` header. + """ + # Validate token (header, hmac.compare_digest) if CONDUCTOR_GATE_TOKEN is set + # Reject (409) if no gate waiting or agent_name != waiting agent + ... + self._gate_response_queue.put_nowait(body) + return JSONResponse({"status": "accepted"}) +``` + +This is a simple adapter that puts the same payload onto `_gate_response_queue` that the WebSocket handler at `server.py:326-327` does today. The `wait_for_gate_response()` method (which sets/clears `self._gate_waiting_agent` in a try/finally) is unchanged. + +**New CLI command** in `cli/app.py`: + +```python +@app.command() +def gate_respond( + port: Annotated[int, typer.Option("--port", "-p", help="Dashboard port")], + choice: Annotated[str, typer.Option("--choice", "-c", help="Selected gate option value")], + agent: Annotated[str | None, typer.Option("--agent", "-a", help="Gate agent name")] = None, + input_text: Annotated[str | None, typer.Option("--input", help="Additional input")] = None, + token: Annotated[str | None, typer.Option("--token", help="Auth token")] = None, +): + """Resolve a parked human gate from the command line.""" + import httpx + ... +``` + +The `--port` flag identifies the running dashboard. If `--agent` is omitted, the command queries `/api/status` (new trivial endpoint returning the currently-waiting gate name, if any) to discover it. + +**Security model (Open Question #3 resolution):** + +- When `CONDUCTOR_GATE_TOKEN` env var is set on the workflow process, the `POST /api/gate-respond` endpoint requires a matching token supplied via the `Authorization: Bearer ` header, compared with `hmac.compare_digest`. +- When unset (default), no auth is required. The dashboard binds to `127.0.0.1` by default, which limits the attack surface to local processes. +- This is proportional to the current security posture: `POST /api/stop` and `POST /api/kill` already exist without auth. The gate-respond endpoint follows the same pattern. + +**Run/Resume parity:** No new flag on `run`/`resume` — the gate-respond command operates independently on a running instance. The `--port` flag comes from the PID file or user knowledge. + +#### 6.2.4 Command Resolution (Issue #3) + +**Location:** `src/conductor/executor/script.py:83-118` + +> **Correction (post-review):** The original plan called for blindly replacing +> `/` with `\` on Windows. That was rejected during adversarial review: the swap +> is a deterministic transform that cannot explain an *intermittent* +> `FileNotFoundError`, it can mangle command strings, and the hint checked the +> unrendered `agent.command` template. Empirical testing on Windows confirmed +> that forward-slash paths and extension-less commands already execute fine via +> `create_subprocess_exec`. The shipped fix instead resolves the command against +> PATH/PATHEXT with `shutil.which`, which is non-destructive and adds real value +> (bare-name → executable resolution). + +After template rendering, resolve the command (not args) when it is an absolute +path or a bare name without a separator; relative paths with a separator are +left untouched so they resolve against `working_dir`: + +```python +import os +import shutil + +has_separator = os.sep in rendered_command or ( + os.altsep is not None and os.altsep in rendered_command +) +if os.path.isabs(rendered_command) or not has_separator: + rendered_command = shutil.which(rendered_command) or rendered_command +``` + +Improve the `FileNotFoundError` handler: + +```python +except FileNotFoundError as exc: + hint = "" + if sys.platform == "win32": + hint = ( + " Hint: on Windows, include the file extension (e.g. .exe) " + "or use an absolute path." + ) + raise ExecutionError( + f"Script '{agent.name}': command not found: '{rendered_command}'" + f" (working_dir={rendered_working_dir or 'cwd'}){hint}", + agent_name=agent.name, + suggestion=f"Ensure '{rendered_command}' is installed and on PATH", + ) from exc +``` + +### 6.3 Design Decisions + +| Decision | Rationale | Alternatives Considered | Status | +|----------|-----------|------------------------|--------| +| `output_mode` is additive with `None` default (Open Q #1 → option (a)) | Preserves full backward compat. Existing workflows continue unchanged. No deprecation churn. | (b) additive + warn — rejected as too heuristic-dependent; (c) default-flip in v0.2.x — too disruptive for a point release. | ✅ SHIPPED | +| Parse-exhaustion marked `is_retryable=False` (Open Q #4 → simpler option) | Deterministic failures don't benefit from retries. Users opt in via YAML `retry.max_attempts`. | Smarter classifier that detects same-error-twice — more complex, fragile, and solves the same problem less transparently. | ✅ SHIPPED | +| Per-agent `max_parse_recovery_attempts` on `RetryPolicy` with `None` default (Open Q #2 → single field, provider defaults preserved) | A single field that both providers respect. `None` means "use provider default" so Copilot=5 and Claude=2 remain the out-of-box experience. | Separate per-provider defaults in YAML — over-engineered; users shouldn't need to know which provider they're targeting. | ✅ DONE | +| Gate-respond via HTTP POST, not WebSocket (Open Q #3) | HTTP POST is simpler for CLI tooling (`httpx` one-shot). The existing WebSocket path continues to work for the dashboard. | WebSocket-only — would require the CLI to maintain a persistent connection, which is overkill for a one-shot operation. | ✅ DONE | +| Token auth is opt-in via env var, not mandatory; supplied via `Authorization: Bearer` header and compared with `hmac.compare_digest` (post-review) | Matches current security posture (`POST /api/stop` has no auth). Avoids breaking changes for localhost-only deployments. Header + constant-time compare avoids body-borne secrets and timing leaks. | Mandatory token — would break existing `--web-bg` setups that don't set env vars. | ✅ DONE | + +--- + +## 7. Dependencies + +### External Dependencies + +- **httpx** — for the `gate-respond` CLI command to POST to the dashboard. Already a direct dependency at `pyproject.toml:46` (`httpx>=0.27.0`). No addition needed. + +### Internal Dependencies + +- **EPIC 1** (output_mode + retry fixes) is **SHIPPED**. No dependencies remain. +- **EPIC 2** (max_parse_recovery in YAML) depends on EPIC 1 for the `is_retryable=False` fix (otherwise the retry budget change would be undermined by outer retries). EPIC 1 is done, so EPIC 2 can proceed immediately. +- **EPIC 3** (gate-respond) is independent of EPICs 1-2. +- **EPIC 4** (Windows paths) is independent of all other epics. + +### Sequencing Constraints + +- EPIC 1 is shipped. The `is_retryable=False` prerequisite for EPIC 2 is satisfied. +- EPICs 2, 3, and 4 are fully independent and can be parallelized across PRs. + +--- + +## 8. Impact Analysis + +### Components Affected + +| Component | Change Type | Risk | Status | +|-----------|-------------|------|--------| +| `config/schema.py` | New field on `AgentDef`, new field on `RetryPolicy` | Low — additive, validated | `output_mode` ✅ DONE; `max_parse_recovery_attempts` ✅ DONE | +| `providers/copilot.py` | Skip schema injection for `raw`, mark parse-exhaustion non-retryable, propagate per-agent parse recovery | Medium — hot path | `output_mode` + `is_retryable` ✅ DONE; parse recovery config ✅ DONE | +| `providers/claude.py` | Mirror all copilot changes per Provider Parity | Medium — hot path | `output_mode` + `is_retryable` ✅ DONE; parse recovery config ✅ DONE | +| `executor/script.py` | `shutil.which` command resolution + error message | Low — non-destructive, cross-platform | DONE | +| `web/server.py` | New HTTP endpoint (with `agent_name` 409 validation + header/hmac token auth) | Low — additive | ✅ DONE | +| `cli/app.py` | New command | Low — additive | ✅ DONE | +| `executor/output.py` | No changes needed | None | — | + +### Backward Compatibility + +- **`output_mode`**: ✅ Shipped. Default `None` preserves current behavior. No existing YAML breaks. +- **`is_retryable=False`**: ✅ Shipped. Workflows that relied on outer-retry-after-parse-exhaustion (unlikely intentional) now fail after inner recovery exhausts. Mitigation: set `retry.max_attempts: 3` to restore old behavior. +- **`max_parse_recovery_attempts`**: Default `None` = provider default. No existing YAML breaks. +- **Gate-respond endpoint**: Additive. No existing clients break. +- **Command resolution**: `shutil.which` is cross-platform and non-destructive — unresolved commands fall through to the original rendered value. + +--- + +## 9. Security Considerations + +### Gate-Respond Endpoint (Issue #5) + +The new `POST /api/gate-respond` endpoint creates a control plane surface: + +- **Default posture**: Dashboard binds to `127.0.0.1`. Gate-respond is local-only, same as `POST /api/stop` and `POST /api/kill`. +- **Shared infrastructure**: When `CONDUCTOR_GATE_TOKEN` is set, the endpoint validates a bearer token. This prevents unauthorized gate resolution in multi-user environments. +- **No escalation path**: Gate-respond submits a choice from the pre-defined options list. It cannot inject arbitrary commands or modify workflow state beyond gate resolution. + +### `output_mode: raw` + +- No security impact. Raw mode returns the model's text as-is without parsing — this is *less* code executed, not more. + +--- + +## 10. Risks and Mitigations + +| Risk | Likelihood | Impact | Mitigation | +|------|-----------|--------|------------| +| `is_retryable=False` breaks workflows that accidentally depended on outer-retry-for-parse | Low | Medium | Document in CHANGELOG; provide `retry.max_attempts` opt-in | +| `output_mode` naming confusion with `output:` | Low | Low | Clear docstring and YAML validation error messages | +| Gate-respond endpoint used for unauthorized gate resolution | Low | Medium | Token auth when `CONDUCTOR_GATE_TOKEN` is set | +| Windows path normalization breaks non-path args containing `/` | Low | Low | Only normalize `rendered_command`, not args. Args may contain URL-like values or flags with `/` and must not be altered. | + +--- + +## 11. Open Questions (from Brainstorm) — Resolutions + +| # | Question | Resolution | +|---|----------|------------| +| 1 | Output mode default: additive vs additive+warn vs default-flip? | **Additive with `None` default.** No warn heuristic (too fragile), no default-flip (too disruptive). Revisit in v0.2.x if adoption data warrants. | +| 2 | Provider parity for retry budget: single value or per-provider defaults? | **Single YAML field, provider defaults preserved via `None`.** When `max_parse_recovery_attempts` is unset, Copilot uses 5 and Claude uses 2. When set, both use the specified value. | +| 3 | Gate-resolution endpoint security? | **Opt-in `CONDUCTOR_GATE_TOKEN` env var.** Proportional to current security posture. No mandatory auth for localhost. | +| 4 | Smarter retry classifier vs per-agent knob? | **Per-agent `is_retryable=False` on parse-exhaustion + YAML `retry.max_attempts` for opt-in.** Simpler, more transparent, no false-positive risk from heuristic classifier. | + +--- + +## 12. Implementation Phases + +### Phase 1: Output Mode + Retry Fixes (Issues #2, #10) — ✅ SHIPPED + +**Exit criteria (met):** A workflow declaring `output_mode: raw` on a large-output agent completes without parse-recovery loops. Parse-exhaustion failures do not trigger outer retries. All tests pass. + +### Phase 2: YAML-Configurable Parse Recovery (Issue #6) — ✅ SHIPPED + +**Exit criteria:** `max_parse_recovery_attempts` is configurable from YAML `retry:` block. Per-agent value reaches the parse recovery loop in both providers. Provider defaults preserved when field is omitted. + +### Phase 3: CLI Gate-Respond (Issue #5) — DONE + +**Exit criteria:** `conductor gate-respond --port --choice ` resolves a parked gate. Token auth works when `CONDUCTOR_GATE_TOKEN` is set. + +### Phase 4: Windows Path Normalization (Issue #3) — DONE + +**Exit criteria:** A script step with `command: "C:/Python314/python.exe"` succeeds on Windows without manual backslash workaround. + +--- + +## 13. Files Affected + +### New Files + +| File Path | Purpose | +|-----------|---------| +| `tests/test_providers/test_parse_recovery_config.py` | Tests for per-agent `max_parse_recovery_attempts` propagation | +| `tests/test_cli/test_gate_respond.py` | CLI gate-respond command tests | +| `tests/test_web/test_gate_respond_api.py` | HTTP gate-respond endpoint tests | + +### Modified Files + +| File Path | Changes | +|-----------|---------| +| `src/conductor/config/schema.py` | Add `max_parse_recovery_attempts` to `RetryPolicy` (after line 395) | +| `src/conductor/providers/copilot.py` | Propagate per-agent `max_parse_recovery_attempts` in `_resolve_retry_config` (line 303), thread resolved config into `_execute_sdk_call` for the parse recovery loop (line 681) | +| `src/conductor/providers/claude.py` | Propagate per-agent `max_parse_recovery_attempts` in `_resolve_retry_config` (line 685), add parameter to `_execute_with_parse_recovery` (line 1738) to accept resolved value instead of reading instance variable (line 1813) | +| `src/conductor/executor/script.py` | Add Windows path normalization after line 86, improve `FileNotFoundError` message at line 113 | +| `src/conductor/web/server.py` | Add `POST /api/gate-respond` endpoint (after line 234), add `/api/gate-status` endpoint | +| `src/conductor/cli/app.py` | Add `gate_respond` command, update `--web-bg` + `human_gate` message text (line 191) | +| ~~`pyproject.toml`~~ | ~~Add `httpx` to dependencies~~ — **No change needed:** `httpx>=0.27.0` already present at line 46 | +| `CHANGELOG.md` | Document `max_parse_recovery_attempts`, `gate-respond`, and Windows path normalization | +| `docs/workflow-syntax.md` | Document `retry.max_parse_recovery_attempts` field with examples | + +### Deleted Files + +| File Path | Reason | +|-----------|--------| +| (none) | | + +--- + +## 14. Implementation Plan + +### EPIC 1: `output_mode` Field + Parse-Exhaustion Retry Fix — ✅ SHIPPED + +**Goal:** Add `output_mode: raw | envelope` to `AgentDef` and fix the outer-retry amplification on parse-exhaustion failures. + +**Prerequisites:** None + +**All tasks verified as implemented in the current codebase:** + +| Task ID | Type | Description | Files | Status | +|---------|------|-------------|-------|--------| +| E1-T1 | IMPL | `output_mode: Literal["raw", "envelope"] \| None = None` field on `AgentDef` with docstring and model_validator rules (raw+output → error, reject on script/human_gate/workflow). | `config/schema.py:508-523, 717-718, 753-754, 782-783, 798-802` | DONE | +| E1-T2 | TEST | Unit tests for `output_mode` validation: raw+no output, envelope+output, raw+output→error, raw on script→error, raw on human_gate→error, raw on workflow→error, None+output, None+no output, envelope+no output, invalid value. | `tests/test_config/test_output_mode.py` (10 tests) | DONE | +| E1-T3 | IMPL | **Copilot provider**: `has_schema = agent.output and agent.output_mode != "raw"` gates schema injection (line 524). Raw path wraps as `{"result": ...}` (lines 670-678). | `providers/copilot.py` | DONE | +| E1-T4 | IMPL | **Claude provider**: `has_schema = agent.output is not None and agent.output_mode != "raw"` gates `emit_output` tool injection (line 922). When `not has_schema`, `output_schema=None` is passed to `_execute_with_parse_recovery` (line 1460). | `providers/claude.py` | DONE | +| E1-T5 | TEST | Provider tests: raw wraps as result, no schema in prompt, envelope backward compat, parse-exhaustion `is_retryable=False`, no outer retry triggered. Both providers. | `tests/test_providers/test_output_mode.py` (10+ tests) | DONE | +| E1-T6 | IMPL | **Copilot**: `is_retryable=False` on parse-exhaustion (line 748). Suggestion text mentions `output_mode: raw` (lines 745-746). | `providers/copilot.py` | DONE | +| E1-T7 | IMPL | **Claude**: `is_retryable=False` on parse-exhaustion (line 1887). Suggestion text mentions `output_mode: raw` (lines 1884-1885). Note: Claude's outer retry uses `_is_retryable_error()` isinstance checks (lines 713-769) which would return `False` for any `ProviderError` regardless; the explicit flag is for clarity and forward compatibility. | `providers/claude.py` | DONE | +| E1-T8 | IMPL | **Copilot**: 500-char error prefix on parse-exhaustion (`response_content[:500]` at line 744) and in `_extract_json` (`content[:500]` at line 1122). | `providers/copilot.py` | DONE | +| E1-T9 | TEST | Regression tests: parse-exhaustion produces `is_retryable=False`, no outer retry triggered. Both providers. | `tests/test_providers/test_output_mode.py` | DONE | +| E1-T10 | IMPL | Documentation: `output_mode` in `docs/workflow-syntax.md:159-203` with examples, `CHANGELOG.md:11-25` with migration note. | `docs/workflow-syntax.md`, `CHANGELOG.md` | DONE | + +**Acceptance Criteria (all met):** +- [x] `output_mode: raw` agent produces `{"result": ...}` with no parse recovery +- [x] `output_mode: raw` + `output:` declared → validation error +- [x] Parse-exhaustion `ProviderError` has `is_retryable=False` +- [x] Error prefix shows first 500 chars of response (both `_execute_sdk_call` and `_extract_json`) +- [x] All existing tests pass (no regressions) +- [x] Provider parity: copilot and claude behave identically for `output_mode` semantics + +### EPIC 2: YAML-Configurable `max_parse_recovery_attempts` — ✅ SHIPPED + +**Goal:** Expose `max_parse_recovery_attempts` in the YAML schema so workflow authors can tune or disable parse recovery per agent. + +**Prerequisites:** EPIC 1 (✅ SHIPPED — parse-exhaustion is non-retryable; this epic provides the correct knob) + +| Task ID | Type | Description | Files | Status | +|---------|------|-------------|-------|--------| +| E2-T1 | IMPL | Add `max_parse_recovery_attempts: int \| None = Field(default=None, ge=0, le=10)` to `RetryPolicy` in `config/schema.py` with docstring. | `config/schema.py` | DONE | +| E2-T2 | IMPL | **Copilot**: In `_resolve_retry_config` (~line 296-304), when building `RetryConfig` from `RetryPolicy`, use `retry.max_parse_recovery_attempts` if not None, else fall back to `self._retry_config.max_parse_recovery_attempts`. **Additionally**, thread the resolved config into the parse recovery loop: update `_execute_with_retry` (~line 335) to pass the resolved `config` to `_execute_sdk_call` as a new parameter (e.g., `retry_config=config`). Update `_execute_sdk_call` signature to accept `retry_config: RetryConfig | None = None`, and at line 681 change `max_recovery = self._retry_config.max_parse_recovery_attempts` to `max_recovery = (retry_config or self._retry_config).max_parse_recovery_attempts`. Without this threading, the resolved per-agent value never reaches the parse recovery loop. | `providers/copilot.py` | DONE | +| E2-T3 | IMPL | **Claude**: Mirror E2-T2 in `_resolve_retry_config` (~line 678-686). Also update `_execute_with_parse_recovery` (line 1738) to accept a `max_parse_recovery_attempts: int` parameter instead of reading from `self._max_parse_recovery_attempts` (instance variable, line 191). Update the call sites in the agentic loop (lines 1404 and 1460) to pass the resolved value from the retry config. The resolved config is available at `_execute_with_retry` (line 873) but currently is not threaded through `_execute_agentic_loop` → `_execute_with_parse_recovery`. | `providers/claude.py` | DONE | +| E2-T4 | TEST | Schema tests: (a) `max_parse_recovery_attempts: 0` → valid, (b) `max_parse_recovery_attempts: 10` → valid, (c) `max_parse_recovery_attempts: -1` → validation error, (d) `max_parse_recovery_attempts: 11` → validation error, (e) omitted → None (provider default). | `tests/test_config/` | DONE | +| E2-T5 | TEST | Provider tests: (a) agent with `retry.max_parse_recovery_attempts: 2` → copilot uses 2 (not default 5), (b) agent with `retry.max_parse_recovery_attempts: 0` → no parse recovery attempted, (c) agent without the field → copilot uses 5, claude uses 2. Both providers. | `tests/test_providers/test_parse_recovery_config.py` | DONE | +| E2-T6 | IMPL | Update `docs/workflow-syntax.md` retry section with `max_parse_recovery_attempts` docs and example. | `docs/` | DONE | + +**Acceptance Criteria:** +- [x] `retry.max_parse_recovery_attempts: 0` disables parse recovery +- [x] Provider defaults preserved when field is omitted (Copilot=5, Claude=2) +- [x] Per-agent value overrides provider default for both providers +- [x] Validation rejects out-of-range values + +### EPIC 3: CLI Gate-Respond Command — ✅ SHIPPED + +**Goal:** Allow users to resolve parked gates from the command line when the dashboard is unreachable. + +**Prerequisites:** None (independent of EPICs 1-2) + +| Task ID | Type | Description | Files | Status | +|---------|------|-------------|-------|--------| +| E3-T1 | IMPL | Add `POST /api/gate-respond` endpoint to `web/server.py`. Accepts JSON body `{agent_name, selected_value, additional_input?}`. Validates `CONDUCTOR_GATE_TOKEN` via the `Authorization: Bearer` header using `hmac.compare_digest` when set. Returns `409` when no gate is waiting or `agent_name` doesn't match the waiting gate. Puts payload onto `_gate_response_queue`. | `web/server.py` | DONE | +| E3-T2 | IMPL | Add `GET /api/gate-status` endpoint to `web/server.py`. Returns JSON `{waiting: bool, agent_name: str?}` reflecting whether a gate is currently waiting. Requires the engine to set a flag on the dashboard when a gate is entered/exited. | `web/server.py` | DONE | +| E3-T3 | IMPL | Add `gate-respond` command to `cli/app.py`. Options: `--port` (required), `--choice` (required), `--agent` (optional, auto-discovered via `/api/gate-status`), `--input` (optional additional text), `--token` (optional auth token, also reads from `CONDUCTOR_GATE_TOKEN` env). Uses `httpx.post` to `http://127.0.0.1:/api/gate-respond`. | `cli/app.py` | DONE | +| E3-T4 | — | ~~Add `httpx` to `pyproject.toml` dependencies.~~ **No-op:** `httpx>=0.27.0` is already a direct dependency at `pyproject.toml:46`. No action needed. | `pyproject.toml` | DONE | +| E3-T5 | TEST | Unit tests for `POST /api/gate-respond`: (a) valid request → 200 + payload on queue, (b) missing `selected_value` → 422, (c) token mismatch via `Authorization` header when `CONDUCTOR_GATE_TOKEN` set → 403, (d) no token required when env var unset → 200, (e) token in JSON body is rejected, (f) no gate waiting / `agent_name` mismatch → 409. | `tests/test_web/test_gate_respond_api.py` | DONE | +| E3-T6 | TEST | CLI tests for `gate-respond` command: (a) happy path with mock server, (b) unreachable port → clear error, (c) token passed from `--token` and from env var. | `tests/test_cli/test_gate_respond.py` | DONE | +| E3-T7 | IMPL | Update `cli/app.py` line 191 text: change "Wait for CLI gate-resolution support (planned follow-up)" → "Use `conductor gate-respond --port --choice ` to resolve from CLI". | `cli/app.py` | DONE | + +**Acceptance Criteria:** +- [x] `conductor gate-respond --port 8080 --choice approve` resolves a parked gate +- [x] Token auth rejects unauthorized requests when `CONDUCTOR_GATE_TOKEN` is set +- [x] Auto-discovery of gate agent name via `/api/gate-status` works +- [x] Error messages are clear when port is unreachable or no gate is waiting +- [x] `--web-bg` + `human_gate` error message references the new command + +### EPIC 4: Command Resolution — ✅ SHIPPED + +**Goal:** Resolve bare command names and absolute paths in script `command:` against PATH/PATHEXT via `shutil.which`, and emit a clearer `FileNotFoundError` message. + +**Prerequisites:** None (independent) + +> **Correction (post-review):** The original tasks below called for blindly +> replacing `/` with `\` on Windows. That deterministic transform was rejected +> during adversarial review (it cannot explain an *intermittent* failure, can +> mangle command strings, and the hint checked the unrendered template). The +> shipped fix resolves the command via `shutil.which` instead — non-destructive +> and adds bare-name → executable resolution. Empirical testing confirmed +> forward-slash and extension-less commands already run fine. + +| Task ID | Type | Description | Files | Status | +|---------|------|-------------|-------|--------| +| E4-T1 | IMPL | In `script.py`, after rendering `rendered_command`, resolve it via `shutil.which` when it is an absolute path or a bare name (no separator); leave relative paths with a separator for `working_dir` resolution; fall back to the rendered value when unresolved. Only resolve `rendered_command`, never args. | `executor/script.py` | DONE | +| E4-T2 | IMPL | Improve `FileNotFoundError` handler: include the resolved command, working directory, and (on Windows) a hint to include the file extension or use an absolute path. | `executor/script.py` | DONE | +| E4-T3 | TEST | Unit tests (patching `shutil.which`): bare name resolved via `which`, absolute path resolved via `which`, `which` returning None falls back to rendered, relative path with separator not resolved, args not resolved, `FileNotFoundError` message includes the Windows hint, no hint on Linux. | `tests/test_executor/test_script.py` | DONE | + +**Acceptance Criteria:** +- [x] Bare command names and absolute paths are resolved against PATH/PATHEXT via `shutil.which` +- [x] Unresolved commands fall through unchanged; relative paths with a separator are left for `working_dir` +- [x] `FileNotFoundError` message includes resolved command and Windows-specific hint +- [x] Args are not resolved (may contain `/` for legitimate purposes) + +--- + +## 15. References + +- [external-workflow-friction.brainstorm.md](external-workflow-friction.brainstorm.md) — source analysis with Phase 1/2/3 validation evidence +- [external-workflow-friction.plan.md](external-workflow-friction.plan.md) — prior plan (v1), shipped in v0.1.17 +- [AGENTS.md](../../../AGENTS.md) — architecture overview, Provider Parity rule, Run/Resume Parity rule +- `src/conductor/config/schema.py` — `AgentDef` (line 450), `output_mode` (line 508), `RetryPolicy` (line 359), `OutputField` (line 61), `validate_agent_type` (line 700) +- `src/conductor/providers/copilot.py` — `RetryConfig` (line 59), `_resolve_retry_config` (line 278), `_execute_with_retry` (line 306), `has_schema` check (line 524), parse recovery loop (line 681), parse-exhaustion error (line 740, `is_retryable=False` at line 748), `_extract_json` (line 1081, truncation at line 1122) +- `src/conductor/providers/claude.py` — `RetryConfig` (line 85), `_resolve_retry_config` (line 660), `_execute_with_retry` (line 834), `_is_retryable_error` (line 713, now honors `ProviderError.is_retryable` first, then falls back to isinstance-based SDK checks), `has_schema` check (line 922), `_execute_with_parse_recovery` (line 1738), parse-exhaustion error (line 1878, `is_retryable=False` at line 1887), `_max_parse_recovery_attempts` instance variable (line 191) +- `src/conductor/executor/script.py` — `create_subprocess_exec` (line 105), `FileNotFoundError` handler (line 113) +- `src/conductor/web/server.py` — gate response queue (line 85), WebSocket handler (line 326), `wait_for_gate_response` (line 712) +- `src/conductor/cli/app.py` — `_abort_web_bg_if_human_gate` (line 158), command definitions +- `src/conductor/cli/pid.py` — PID file schema for port discovery +- `tests/test_config/test_output_mode.py` — 10 schema validation tests for `output_mode` +- `tests/test_providers/test_output_mode.py` — provider behavior tests for `output_mode` and `is_retryable=False` diff --git a/docs/projects/usability-features/external-workflow-friction.brainstorm.md b/docs/projects/usability-features/external-workflow-friction.brainstorm.md index ea33fd5e..b7042616 100644 --- a/docs/projects/usability-features/external-workflow-friction.brainstorm.md +++ b/docs/projects/usability-features/external-workflow-friction.brainstorm.md @@ -1,32 +1,42 @@ # External Workflow Friction — Findings and Fix Brainstorm -> **Status:** brainstorm — open for discussion before any of the proposals here become a `.plan.md`. +> **Status:** brainstorm — updated 2026-05-27 with validation evidence against Conductor v0.1.17. > **Author:** Lucio Tinoco (external user contributor) -> **Source of evidence:** real-world execution of the `workiq-coach` Conductor workflow set (Phase A: WorkIQ fan-out → synthesizer → schema validator → save; Phase B: four parallel artifact generators → consistency check → 5 saves). Seven failed runs across May 25–26, 2026, before a successful end-to-end execution was achieved. +> **Source of evidence:** real-world execution of the `workiq-coach` Conductor workflow set, then a three-phase validation pass against v0.1.17. See *Validation results* below. > **Audience:** Conductor maintainers, and any contributor who'd help upstream these fixes. --- ## Why this document exists -A single user trying to run two non-trivial workflows against real WorkIQ + Copilot + Anthropic data hit nine distinct rough edges in Conductor v0.1.16. Each one looked like a one-off the first time it appeared; each turned out to be a real bug or design fragility. Most are small fixes. A few are architectural choices worth discussing. +A single user trying to run two non-trivial workflows against real WorkIQ + Copilot + Anthropic data hit nine distinct rough edges in Conductor v0.1.16. Each one looked like a one-off the first time it appeared; each turned out to be a real bug or design fragility — or, in two cases, an analysis error on my part. This document records what was learned so the cost of that session pays back across the codebase rather than being a private tale. It is **not** a request to fix everything at once — it's a structured analysis with a phased implementation plan that maintainers can carve into PR-sized work, reshape, reject, or defer. The unifying theme: each issue produces a **silent or confusing failure mode** that consumes minutes-to-hours of an external user's time before the actual cause becomes diagnosable. The fixes are mostly about **earlier and clearer errors**, **safer defaults**, and **eliminating brittle implicit behaviors**. +Since the first revision of this doc, Conductor v0.1.17 has shipped fixes that **fully address** three of the nine original items (#1 small-case, #7, #8), **partially address** two more (#5, #6), leave **two open** (#2, #3), and turn out to **not be bugs** for two more (#4 and #9 — closer inspection during validation showed I was wrong about both). A **tenth** issue surfaced during validation (#10, agent-level outer retry). Each section below now carries a status badge reflecting the v0.1.17 state. + --- ## Executive summary -| Tier | Theme | Issues | PR cluster | -|---|---|---|---| -| **1** | Silent/confusing failures → clear errors | #4 var expansion, #7 script in parallel, #8 web-bg + gate, parts of #3 | "Better validation + error messages" | -| **2** | Architectural fragility in JSON envelope extraction | #1 fence regex, #2 `output:` schema default | "Output mode: raw vs envelope" | -| **3** | Operational reliability | #3 subprocess intermittency, #5 dashboard zombie, #6 retry budget, #9 expansion parity | "Runtime hardening" | -| **Bonus** | Patterns that look fragile but haven't surfaced in our use yet | event-history unbounded growth, parallel context race, recovery prompt reuses identical schema, no dashboard-startup timeout | Future cleanup | +| # | Issue | v0.1.17 status | +|:---:|---|:---| +| 1 | Fence-extraction regex non-greedy | ⚠️ **PARTIAL** — greedy regex shipped at `output.py:120-122`; works on small responses (Phase 2 ✓), fails on full-scale ~80 KB responses (Phase 3 ✗). Issue #2 still needed. | +| 2 | `output_mode: raw \| envelope` field on AgentDef | ❌ **STILL OPEN** — Phase 3 empirically validated this as essential, not just nice-to-have. Headline upstream contribution candidate. | +| 3 | Windows subprocess path normalization | ❌ Still open — workaround: set `$env:PYTHON` with backslash form. | +| 4 | `${VAR:-DEFAULT}` colon-in-default parser | ❎ **DEBUNKED** — empirical test against v0.1.17 (and v0.1.16) shows the regex correctly parses `${PYTHON:-C:/Python314/python.exe}`. My original analysis was wrong. This issue does not exist; the section is preserved below as a note for future readers. | +| 5 | Dashboard / gate-resolution resilience | ⚠️ **PARTIAL** — `web/server.py` now handles `gate_response` / `dialog_message` / `iteration_limit_response` messages and has disconnect-event handling; PR #202 brought max-iterations gate resolution into the dashboard. No CLI `conductor gate-respond` command yet. | +| 6 | Per-agent retry budget config | ⚠️ **PARTIAL** — `max_parse_recovery_attempts` is now on an internal `_retry_config` (configurable in code), but not surfaced in the YAML schema. | +| 7 | Reject `type: script` in `parallel:` at validate | ✅ **SHIPPED** — `config/validator.py:489-492` rejects with: *"Script steps cannot be used in parallel groups."* | +| 8 | `--web-bg` + `human_gate` clear error | ✅ **SHIPPED VERBATIM** — `cli/app.py:158-191` defines `_abort_web_bg_if_human_gate` with essentially the brainstorm's proposed error message word-for-word, four-option list intact. | +| 9 | `command:` and `args:` expansion parity | ❎ **NOT A BUG** — confirmed both use the same `self.renderer.render()` path in `executor/script.py`. No divergence to fix. | +| 10 | Agent-level outer retry budget (new finding) | ❌ **STILL OPEN** — Phase 3 surfaced this: when parse-recovery 5/5 exhausts, Conductor retries the whole agent 3 more times. Multiplies sunk cost on doomed agents. Should be capped/configurable. | + +**Headline remaining upstream priority:** Issue #2 (the `output_mode` field). This is the architectural fix that the regex patch (Issue #1) cannot substitute for. Phase 3 verified empirically. -The single highest-leverage change is **#1 + #2 together** — the JSON envelope extraction + the `output:` schema default. These are responsible for the majority of "Parse Recovery 1/5 → 5/5 → workflow times out" experiences we hit. Tier 1 fixes are individually cheap and add up to a much smoother first-run experience for new external users. +**Headline upstream wins already in v0.1.17:** Issues #1 (small-case), #5 (partial), #7, #8. Cluster A from the first revision of this doc is largely shipped. --- @@ -48,12 +58,53 @@ For Phase B (which already used the no-`output:`-schema pattern from the start), **Net loss to friction across runs #1–#7: roughly 3 hours of wall-clock LLM execution + several hours of human diagnostic time, almost all attributable to issues #1, #2, and #5.** -The other six issues (#3, #4, #6, #7, #8, #9) surfaced in supporting fashion — each cost minutes, and each is independently a real bug. +The other six issues (#3, #4, #6, #7, #8, #9) surfaced in supporting fashion — each cost minutes, and each is independently a real bug (except #4 and #9, which the validation pass below proved are not actually bugs). + +--- + +## Validation results (2026-05-27, against Conductor v0.1.17) + +After v0.1.17 shipped, a three-phase validation pass tested whether the fixes addressed the original failure modes. Summary above; details: + +### Phase 1 — Static survey + +Grep + read each cited file:line in v0.1.17. Result: the matrix in the executive summary. Five issues had relevant code changes (three full ships, two partials). Two issues turned out to be analysis errors. Two are still open. + +Most striking find: `cli/app.py:158-191` contains the **exact error message** I proposed in this brainstorm for Issue #8 — four-option list intact down to the "Wait for CLI gate-resolution support (planned follow-up)" line. Strong signal that the doc was read and acted on. + +### Phase 2 — Minimal repro for Issue #1 (greedy regex) + +A 5-line repro YAML with one agent declaring `output: { content: string }` and a prompt that asks the model to emit a string containing triple-backticks. Against v0.1.17: + +- Workflow completed in **14.35 seconds** +- **Zero Parse Recovery events** +- Output verified: `"contains_backticks": "True"` (the test was real, not degenerate) +- Cost: $0.0053 + +The greedy regex at `output.py:120-122` works as designed for small-case nested fences. This **confirms Issue #1 was correctly fixed** for the small-response case. + +### Phase 3 — End-to-end against the original workiq-coach config + +Restored `phase-a.yaml`'s synthesizer to the **original** `output: { observations_json, pillar_summary }` schema — the config that broke 5+ times against v0.1.16 — and ran against v0.1.17 with `budget_usd: 2.00` in `enforce` mode as a safety net. + +Result: **synthesizer still parse-loops at full scale.** Same failure mode as v0.1.16 runs #2/#4/#6: + +- 8 workiq_runner agents completed cleanly (each with one transient parse recovery, all recovered) +- Synthesizer step started; first attempt's response hit Parse Recovery 1/5 → 2/5 → 3/5 → 4/5 → 5/5 +- Conductor then surfaced a new behavior I hadn't seen before: an **agent-level outer retry budget** (3 attempts total). After parse recovery 5/5, the message was `Agent 'synthesizer' attempt 1/3 failed: Failed to parse structured output from agent response`, and the inner parse-recovery cycle restarted from 1/5 inside outer attempt 2/3. +- Killed at ~50 min runtime, mid outer-attempt 2/3 +- Budget enforcement did **not** fire — but only because actual spend stayed under $2 (~$0.70-0.80 estimated). The budget feature is working correctly; haiku is just too cheap for $2 to brake a retry storm of this size. + +**Conclusion:** Issue #1's greedy regex fix is verified to work at small scale but **does not address the full-scale failure**. Something else about the synthesizer's ~80KB response trips Conductor's parser — likely model truncation crossing the fence boundary, or prose interleaved with the JSON. Issue #2 (`output_mode: raw | envelope`) remains the architectural fix that this validation pass empirically demands. + +Total cost of validation: ~$0.81 across Phase 1 (free), Phase 2 ($0.005), and Phase 3 (~$0.80). --- ## Issue 1 — Fence-extraction regex breaks on large or nested JSON +> **Status (v0.1.17):** ⚠️ PARTIAL. Greedy regex shipped at `executor/output.py:120-122` with a comment that quotes this brainstorm's failure mode. Phase 2 verified the fix on a small response. Phase 3 verified the fix is **insufficient** at full scale (~80 KB synthesizer output). Issue #2 below remains the architectural fix. + ### Symptom When an agent declares an `output:` schema, the provider injects an instruction to "respond with JSON matching this schema." Models — particularly opus and gpt-5.2, and sometimes haiku — wrap large JSON responses in ` ```json ... ``` ` fences. Conductor's extraction regex is non-greedy: it matches the first ` ``` ` it sees after the opening fence. If the JSON content contains backticks (code snippets in `your_excerpt` strings, etc.), or simply if the response is large enough that the model breaks it across the fence boundary, the regex truncates and the extracted JSON is invalid. @@ -88,6 +139,8 @@ The brace-balanced approach is more robust but ~30 LOC of careful code (must han **Recommendation:** ship the greedy regex first as a quick win; add the brace-balanced extractor as a follow-up for the long tail. +> *Update 2026-05-27 — Upstream shipped the greedy-regex change at `output.py:120-122`. The accompanying code comment reproduces this brainstorm's failure-mode description ("closes at the LAST ` ``` ` in the response, not the first inner ` ``` ` which may appear inside a JSON string field"). Phase 2 confirmed it works on small inputs; Phase 3 showed it does **not** scale (see "Update from Phase 3 validation" below). The brace-balanced extractor may still be worth adding, but the upstream priority should be Issue #2.* + ### Blast radius - All workflows with `output:` schemas and large structured responses @@ -97,16 +150,30 @@ The brace-balanced approach is more robust but ~30 LOC of careful code (must han ### Validation approach Add tests under `tests/test_executor/test_output.py`: -- Fence-wrapped JSON with triple-backticks inside a string field -- Fence-wrapped JSON ~80 KB in size +- Fence-wrapped JSON with triple-backticks inside a string field ✅ (Phase 2 confirms this passes) +- Fence-wrapped JSON ~80 KB in size ❌ (Phase 3 demonstrates this still fails) - Fence-wrapped JSON with prose before and after the fence - Raw JSON with no fence - Malformed JSON (must still fail cleanly) +### Update from Phase 3 validation (2026-05-27) + +The 80KB synthesizer case still fails. Possible root causes (not yet root-caused): + +1. **Model response truncation**: At ~80KB, models may emit incomplete output where the closing ` ``` ` is missing. Greedy regex can't recover from a truly absent closing fence. +2. **Prose interleaved with JSON**: When asked to "respond with JSON matching this schema," some models still emit "Here is the synthesized JSON:\n```json\n{...}\n```\nLet me know if..." — prose before AND after the fence. The current extractor handles prose-then-JSON and JSON-then-prose, but not the dual case cleanly. +3. **Field-shape mismatch inside the envelope**: The model may emit valid JSON whose **shape** doesn't match `{observations_json: string, pillar_summary: string}` — e.g. emitting the full observations object directly at top level instead of nesting it as a string in `observations_json`. Conductor's "Could not extract JSON" message may be misleading; the actual failure might be field validation. + +The Conductor error at the parse failure surfaces "Response started with: ..." but truncates to "..." so the actual prefix isn't visible — diagnosis would benefit from a longer prefix in the error message (e.g. first 500 chars). + +**Recommendation for upstream:** even with the greedy regex in place, ship Issue #2's `output_mode: raw` field. The greedy regex helps the small case; only `output_mode: raw` addresses the architectural problem at scale. + --- ## Issue 2 — `output:` schema is the wrong default for prose / large JSON agents +> **Status (v0.1.17):** ❌ STILL OPEN. The `output_mode` field is not in `config/schema.py`. The recent `9d603a1` commit (`feat(script): allow script agents to declare output schemas`) is about *script* agents getting `output:`, which is a different feature. Phase 3 empirically validated that this issue is the **architectural root cause** of the workiq-coach failures. Headline upstream contribution candidate. + ### Symptom `output:` is intuitively the "right" way to declare what an agent produces. New users specify it for every agent. For agents that produce small, strictly-structured JSON, it works fine. For agents that produce: @@ -175,6 +242,8 @@ Add an explicit `output_mode` field to `AgentDef`: ## Issue 3 — Subprocess invocation fails intermittently on Windows forward-slash paths +> **Status (v0.1.17):** ❌ STILL OPEN. No forward-slash → backslash normalization in `executor/script.py`. Did not recur during Phase 3 validation, but workaround remains: set `$env:PYTHON` to a backslash absolute path. + ### Symptom A `type: script` step with `command: "${PYTHON:-python}"`, with `$env:PYTHON = "C:/Python314/python.exe"`, fails with: @@ -222,54 +291,32 @@ Normalize separators on Windows. Also improve the error message: when `FileNotFo ## Issue 4 — `${VAR:-DEFAULT}` regex splits on the first `:` in the default -### Symptom - -`${PYTHON:-C:/Python314/python.exe}` fails to expand correctly: the first `:` (after `C`) is misread as the `:-` default separator, producing nonsensical variable resolution. Forces users to either set `$env:PYTHON` to absolute and use `${PYTHON:-python}` (with a colon-free default), or to avoid env-var defaults entirely for Windows paths. - -### Location - -- `src/conductor/config/loader.py:23` — env var expansion regex +> **Status (v0.1.17 and v0.1.16):** ❎ **DEBUNKED — this is not a bug.** The original brainstorm claim was wrong. +> +> The regex at `config/loader.py:23` is `r"\$\{([^}:]+)(?::-([^}]*))?\}"`. The variable-name portion `[^}:]+` excludes colons (so it can't accidentally absorb `C:`), and the default-value portion `[^}]*` correctly accepts colons. Empirical test: +> +> ``` +> "${PYTHON:-C:/Python314/python.exe}" → VAR='PYTHON', DEFAULT='C:/Python314/python.exe' +> "${WORKIQ_COACH_ROOT:-Q:/src/workiq-coach}" → VAR='WORKIQ_COACH_ROOT', DEFAULT='Q:/src/workiq-coach' +> "${VAR:-default:with:colons}" → VAR='VAR', DEFAULT='default:with:colons' +> ``` +> +> All cases resolve correctly. My original analysis confused the workiq-coach user's notes ("we tried `${PYTHON:-C:/...}` and it didn't work") with a regex bug. The actual problem at the time was almost certainly something else downstream (possibly the subprocess invocation issue from Issue #3). Leaving this section in place as a warning to future readers: validate empirically before proposing fixes to regex-shaped code. -### Cause - -The regex (or equivalent string-split) treats `:` greedily as the var/default separator. Splits on the *first* `:` encountered. Windows drive letters violate this assumption. +### What I originally thought -### Fix proposal +That the regex split on the *first* `:` rather than `:-`, mangling `${PYTHON:-C:/path}` into `VAR=PYTHON, DEFAULT=C` (with the rest discarded). This is not what happens; the regex's variable-name class `[^}:]+` correctly stops at the first `:`, but then `(?::-...)?` requires the literal `:-` sequence (colon-dash) to enter the default group. A bare `:` after the var name does not match the optional default group. -Replace the regex with a parser that scans the token from `${` to `}` and uses `rfind(":-")` to locate the default separator only at the **last** `:-` occurrence: +### Lesson learned -```python -def parse_var_token(token: str) -> tuple[str, str | None]: - """Parse ${VAR:-DEFAULT} content (the text between ${ and }).""" - sep = ":-" - idx = token.rfind(sep) - if idx == -1: - return token, None - return token[:idx], token[idx + len(sep):] -``` - -Walk the source string for `${...}` blocks and apply this parser. - -Alternative: document that defaults can't contain `:` and validate at load time — but the parser fix is small and removes a real footgun. - -### Blast radius - -- Windows users with absolute-path defaults -- POSIX users unaffected (`:` is rare in defaults) -- Backward-compatible (existing defaults without colons still work identically) - -### Validation approach - -- `tests/test_config/test_loader.py` cases: - - `${VAR:-C:/path/with/colons}` resolves to `C:/path/with/colons` when VAR unset - - `${VAR:-default}` still works (no colon) - - `${VAR}` (no default) still works - - Edge: nested `${...}` inside default value +When proposing a regex fix, run the regex against the exact failure input first. Five minutes with `re.compile().search()` would have caught this. --- ## Issue 5 — Dashboard web server dies during long-parked human gates +> **Status (v0.1.17):** ⚠️ PARTIAL. `web/server.py:315-345` now handles `gate_response`, `dialog_message`, and `iteration_limit_response` messages from clients, with `_disconnect_event` and grace timers for connection lifecycle. PR `dc29c2c` (*fix(engine,web): resolve max-iterations gate from dashboard in --web-bg*) brings gate resolution into the dashboard itself. **What's still missing:** a CLI `conductor gate-respond ` command for resolving gates from outside the browser when the dashboard is unreachable. + ### Symptom A workflow with a `human_gate` that sits awaiting user input for many hours (overnight is a realistic case) ends up in a zombie state: @@ -317,6 +364,8 @@ The CLI gate-resolution command is the most impactful single change — it gives ## Issue 6 — Parse-recovery retry budget hardcoded per provider +> **Status (v0.1.17):** ⚠️ PARTIAL. `max_parse_recovery_attempts` has been moved to an internal `_retry_config` field (visible in `providers/copilot.py:685` and `claude.py:191`), but is **not exposed in the workflow YAML schema**. The internal refactor is half the work; the user-facing knob is what would let workflows fail fast on doomed agents (especially with the new outer-retry budget — see Issue #10). + ### Symptom When parse recovery is needed, Copilot gets 5 retries, Claude gets 2 (per the user's notes; values from `copilot.py:81` and `claude.py:106`). These are class-level constants. For large outputs prone to parse failure, 5 may be too few; for short, fast outputs in CI/cost-sensitive contexts, 5 may be too many. @@ -357,6 +406,8 @@ Honors the [Provider Parity](../../AGENTS.md#provider-parity) rule — both prov ## Issue 7 — `type: script` agents inside `parallel:` groups silently misbehave +> **Status (v0.1.17):** ✅ **SHIPPED.** `config/validator.py:489-492` rejects with: *"Agent '\' in parallel group '\' is a script step. Script steps cannot be used in parallel groups."* This is Cluster A's quick-win item from the first revision of this brainstorm. Done. + ### Symptom A YAML like: @@ -410,6 +461,8 @@ Then execute concurrently. Requires careful error handling (`continue_on_error` **Recommendation:** ship A first (1-day fix, no behavior change for valid workflows), then B as a follow-up if there's demand. +> *Update 2026-05-27 — Upstream shipped Option A at `config/validator.py:489-492`. The error message is essentially as proposed. Done.* + ### Blast radius - Workflows that put scripts in parallel groups (currently silently broken) @@ -424,6 +477,8 @@ Then execute concurrently. Requires careful error handling (`continue_on_error` ## Issue 8 — `--web-bg` + `human_gate` crashes with EOFError +> **Status (v0.1.17):** ✅ **SHIPPED — verbatim.** `cli/app.py:158-191` defines `_abort_web_bg_if_human_gate` whose error message reproduces the brainstorm's proposed text essentially word-for-word, including the four-option remediation list ("Use --web (foreground)…", "Add --skip-gates…", "Remove human_gate steps…", "Wait for CLI gate-resolution support (planned follow-up)"). Honors `--skip-gates` as the documented escape hatch. Strong evidence that this brainstorm was read; thank you to whoever picked it up. + ### Symptom Running `conductor run --web-bg` with a workflow that includes a `human_gate` crashes the detached background process with an `EOFError` when the gate prompt tries to read stdin (which is redirected to /dev/null in the detached child). @@ -461,6 +516,8 @@ If `not is_interactive` and the workflow contains gates without `--skip-gates`: ``` 2. **Runtime fallback** (more complex): when a gate fires in detached mode, route it to the dashboard's `/api/gate-response` endpoint (see Issue #5 fix #2) and poll for the response. Requires the gate-respond endpoint to exist. +> *Update 2026-05-27 — Upstream shipped Option 1 verbatim at `cli/app.py:158-191`. The error message reproduces the proposed text including the four-option remediation list. Done.* + ### Blast radius - `--web-bg` + gate workflows (currently crash) @@ -474,41 +531,56 @@ If `not is_interactive` and the workflow contains gates without `--skip-gates`: ## Issue 9 — Possible expansion-path divergence between `command:` and `args:` +> **Status (v0.1.17 and v0.1.16):** ❎ **NOT A BUG.** Confirmed both fields use `self.renderer.render()` on the same code path in `executor/script.py:86-87`. No divergence to fix. Original brainstorm was speculative; validation pass closed it out. +> +> The "anecdotal" observation that motivated this item was confused with Issue #4 (which itself turned out not to be a bug). Both go through the same Jinja2 template rendering. A test verifying parity is still a reasonable defensive addition for `tests/test_executor/test_script.py`, but the issue itself can be closed. + +--- + +## Issue 10 — Agent-level outer retry budget amplifies sunk cost (NEW, from Phase 3) + +> **Status (v0.1.17):** ❌ STILL OPEN. New finding from Phase 3 validation. + ### Symptom -Anecdotal: `${VAR:-default}` expansion appears to behave differently in `command:` vs. `args:` fields of a script step. The user's debugging notes for Issue #4 mention this is what led to the colon-in-default workaround being needed for `command:` but not `args:`. Hasn't been root-caused; may be related to Issue #4 or may be a separate code path. +When an agent's inner parse-recovery cycle (5 attempts) exhausts, Conductor v0.1.17 retries **the whole agent up to 3 more times**. The outer-attempt counter is visible in the log as `Agent 'synthesizer' attempt 1/3 failed: ...`, after which Parse Recovery 1/5 begins again inside outer attempt 2/3. + +This was not visible in v0.1.16 (or at least, I didn't observe it). It looks like a hardening pass that adds resilience to transient failures — but for **deterministic** schema-mismatch failures (the workiq-coach synthesizer case), it triples the sunk cost. ### Location -- `src/conductor/executor/script.py:86-87` — template rendering for both fields +Likely `providers/copilot.py` or `engine/workflow.py` — not yet root-caused. Visible in v0.1.17 log output as `Agent '' attempt N/3 failed: Failed to parse structured output...`. ### Cause -Unknown without deeper investigation. Possible candidates: -1. Different render order (env var resolution at YAML load time vs. Jinja2 template render time) -2. One field passes through a parser that the other doesn't -3. The observation was incorrect and both behave identically once Issue #4 is fixed +Hardening retry logic that doesn't distinguish *transient* failures (worth retrying) from *deterministic* configuration mismatches (won't change on retry). The error message includes `Retryable: True`, but the determination of retryability appears to be based on the failure category, not on whether retrying could actually succeed. ### Fix proposal -Audit `script.py` to confirm both `command:` and `args:` use the same render path. Add unit tests that verify equivalence: +Three options, can ship any subset: -```python -def test_command_and_args_env_var_parity(): - """Same ${VAR:-default} resolves identically in command: and args: fields.""" - # ... test that command="${X:-foo}" and args=["${X:-foo}"] both produce "foo" -``` +1. **Per-agent `max_outer_attempts` config** — let workflow authors opt out of the outer retry for agents known to fail-deterministically: + ```yaml + - name: synthesizer + retry: + max_outer_attempts: 1 # don't burn extra attempts on deterministic failures + max_parse_recovery_attempts: 2 + ``` -If a divergence exists, unify the paths. +2. **Smarter retry classifier** — if the same parse error fires twice in a row inside one outer attempt, mark the failure as deterministic and skip remaining outer attempts. Saves cost without requiring user configuration. + +3. **Surface a deprecation/warning when the outer retry is triggered** — make the cost visible. Many users (myself included) wouldn't notice the 3× spend amplification until reading the bill. ### Blast radius -- Probably small — would have surfaced more widely if significant -- Test coverage improvement is valuable regardless +- Workflows with deterministically-failing agents (e.g. envelope mismatch on large outputs) +- Production runs where cost amplification matters +- Backward-compatible if added as optional config with current behavior as the default ### Validation approach -- New test in `tests/test_executor/test_script.py` +- Integration test: agent configured to always fail parse — count outer attempts, verify budget config caps them +- Cost-tracking test: verify the budget tracker counts outer retries the same as parse recoveries --- @@ -542,62 +614,69 @@ These are things the analysis surfaced as "this will bite someone eventually" bu --- -## Implementation plan — three PR clusters - -### Cluster A: "Better validation + error messages" (highest value-to-effort) +## Implementation plan — revised after v0.1.17 -**Goal:** turn silent and confusing failures into clear errors at validate time or at the failure site. +The first revision of this doc proposed three PR clusters. Phase 1/2/3 validation against v0.1.17 changes the picture significantly: **Cluster A is mostly already shipped**, **Cluster B is now the headline priority**, and **Cluster C shrinks**. -**Includes:** -- Issue #4: `${VAR:-DEFAULT}` parser fix -- Issue #7: reject `type: script` in `parallel:` at validate -- Issue #8: detect non-interactive stdin + workflow gates at startup -- Issue #3: normalize Windows path separators + improve subprocess error message +### Cluster A (largely shipped in v0.1.17 — leftover items only) -**Estimated size:** ~60 LOC across `config/loader.py`, `config/validator.py`, `executor/script.py`, `gates/human.py`. Plus tests. +Original scope included Issues #4, #7, #8, and parts of #3. Updated: -**Risk:** very low. All changes are either pure parser fixes or earlier-validation. No behavioral change for valid workflows. +- ✅ **Issue #7** — shipped (`config/validator.py:489-492`) +- ✅ **Issue #8** — shipped (`cli/app.py:158-191`) +- ❎ **Issue #4** — debunked (not actually a bug) +- ❌ **Issue #3** — Windows path normalization not yet shipped. Still a 10-line PR. -**Why ship first:** every one of these is an instance of "user hits a confusing failure, takes 30+ minutes to diagnose, fix is 2 lines of code." Each PR independently improves the new-user experience. +**Remaining Cluster A scope:** Issue #3 alone (~20 LOC including tests). Trivially mergeable. -### Cluster B: "Output mode: raw vs envelope" (architectural) +### Cluster B: "Output mode: raw vs envelope" — NOW THE HEADLINE PRIORITY -**Goal:** make raw-response the explicit, documented, recommended pattern for agents producing prose or large JSON. +**Goal:** introduce an explicit `output_mode: raw | envelope` field so that prose / large-JSON agents have a documented, first-class way to opt out of the JSON envelope contract that empirically fails at scale. **Includes:** -- Issue #1: greedy fence regex (quick fix) + optional brace-balanced extractor (proper fix) -- Issue #2: add `output_mode: raw | envelope` to AgentDef; warn at validate when `output:` is declared on prose-likely agents; documentation updates +- Issue #1 (partially shipped) — keep the greedy regex; consider the brace-balanced extractor as a follow-up only if Issue #2 doesn't subsume the need +- Issue #2 — add `output_mode: raw | envelope` to `AgentDef`; warn at `conductor validate` when `output:` is declared on prose-likely agents (heuristic on prompt content); documentation updates in `docs/workflow-syntax.md` and `docs/configuration.md` +- Issue #10 (new) — pair with a per-agent `retry.max_outer_attempts` knob. Without this, even with `output_mode: envelope` declared correctly, a transient failure burns 3× the necessary cost. -**Estimated size:** ~100 LOC across `executor/output.py`, `providers/copilot.py`, `providers/claude.py` (parity), `config/schema.py`, `config/validator.py`. Plus docs updates and tests. The fence-regex piece is small; the `output_mode` field + validator warning + doc cohesion is most of the work. +**Empirical justification:** Phase 3 demonstrated that the greedy regex alone is insufficient for the full-scale workflow that originally motivated this brainstorm. The `output_mode: raw` field is the architectural fix, not a workaround. -**Risk:** medium. Affects the hot path of every workflow with `output:`. Needs careful provider-parity work. Worth a design discussion in this brainstorm before opening a PR. +**Estimated size:** ~150 LOC across `config/schema.py`, `config/validator.py`, `executor/output.py` (touch only), `providers/copilot.py` + `providers/claude.py` (parity), plus docs updates and tests. -**Why ship together:** the regex fix without the documented `output_mode` field still leaves new users tripping into the bad default. The field without the regex fix doesn't help existing workflows with valid `output:` declarations that happen to contain backticks. +**Risk:** medium. Affects the hot path of every workflow with `output:`. Backward-compatible if the default behavior is preserved when `output_mode` is unspecified (existing workflows continue to behave as today). Worth a design discussion in this brainstorm before opening a PR — see Open question #1 below. -### Cluster C: "Runtime hardening" +**Why ship this:** this is the architectural fix the validation pass empirically demands. Every other item on this list is comparatively cosmetic. -**Goal:** improve operational reliability of long-running workflows and dashboard interactions. +### Cluster C: "Runtime hardening" — shrunk -**Includes:** -- Issue #5: dashboard WebSocket keepalive + CLI gate-resolution command -- Issue #6: configurable `max_parse_recovery_attempts` -- Issue #9: unified expansion path + parity test -- Bonus: event-history ring buffer, dashboard-startup timeout +Original scope included Issues #5, #6, #9 + bonus patterns. Updated: + +- ⚠️ **Issue #5** — partially shipped. Remaining: CLI `conductor gate-respond ` command for resolving gates outside the browser. ~50 LOC + tests. +- ⚠️ **Issue #6** — partially shipped (internal refactor). Remaining: expose `max_parse_recovery_attempts` in the YAML schema. ~10 LOC + schema test. +- ❎ **Issue #9** — confirmed not a bug. Closed. +- Bonus patterns — still flagged below; out of scope for an immediate PR. + +**Remaining Cluster C scope:** Issue #5 (CLI gate-respond) + Issue #6 (YAML field). ~60 LOC combined. -**Estimated size:** ~200 LOC, primarily in `web/server.py`, `cli/`, `providers/`. Plus the new `conductor gate-accept` CLI command. +### What I'd PR if I were doing this myself -**Risk:** medium-low. The keepalive and ring buffer are additive. The CLI gate-resolution is a net-new feature. +In priority order: -**Why ship last:** these are quality-of-life improvements rather than fixes for immediately-broken behavior. Maintainers may want to defer until A + B prove the brainstorm's value. +1. **Issue #2 + #10** as a single design-discussion-first PR (Cluster B headline). Opens the conversation about the `output_mode` field with empirical Phase 3 evidence. Optionally bundles the `retry.max_outer_attempts` knob. +2. **Issue #3** as a small, focused Windows path normalization PR. Probably mergeable in a day. +3. **Issue #6** as a small YAML schema PR exposing the already-internal retry budget knob. +4. **Issue #5 CLI command** as a small feature PR. --- ## Open questions for maintainers -1. **Output mode default.** Cluster B proposes `output_mode` as an additive field with the existing default preserved. Would maintainers consider flipping the default to `raw` in a future v0.2.x, given that the current default produces parse recovery loops on real-world workflows? (Backward-compat strategy: behave as today when `output_mode` is unspecified AND `output:` is specified; warn loudly via deprecation when this combination appears in `conductor validate`.) -2. **Provider parity for retry budget.** Issue #6 proposes per-agent `retry.max_parse_recovery_attempts`. The current Copilot default is 5; Claude is 2. Should the proposed field be a single value that both providers respect, or should the default per-provider be preserved (5 for Copilot, 2 for Claude) with the per-agent override applying uniformly? -3. **Gate resolution endpoint security.** Adding `POST /api/gate-response` to the dashboard server creates a new attack surface. Should it require a per-run token (passed via env var or CLI flag) to authorize gate responses? The dashboard is bound to localhost by default, but `--web-bg` users running on shared infrastructure might want explicit token-based auth. -4. **Issue #8 fix shape.** Validate-time error vs. runtime dashboard fallback for `--web-bg + human_gate`. The dashboard fallback is the better UX but depends on the gate-response endpoint from Issue #5 existing. Order the work? +1. **Output mode default.** Cluster B proposes `output_mode` as an additive field with the existing default preserved. Phase 3 evidence suggests the current default (envelope-when-`output:`-is-present) produces parse-recovery loops on real-world large-output workflows. Would maintainers consider: + (a) shipping `output_mode` additive with current default preserved, + (b) shipping additive + warning loudly at `conductor validate` when `output:` is declared on a prose-likely agent, OR + (c) flipping the default to `raw` in v0.2.x with a deprecation pass for explicit-envelope workflows? +2. **Provider parity for retry budget.** Issue #6 proposes per-agent `retry.max_parse_recovery_attempts`. The current Copilot default is 5; Claude is 2. Should the proposed field be a single value that both providers respect, or should the default per-provider be preserved (5 for Copilot, 2 for Claude) with the per-agent override applying uniformly? Phase 3 surfaces a related concern: the **outer** retry budget (Issue #10) is also unconfigurable. Worth bundling. +3. **Gate resolution endpoint security.** Adding a CLI gate-resolution surface (Cluster C Issue #5) and/or `POST /api/gate-response` to the dashboard server creates a new attack surface. Should it require a per-run token (passed via env var or CLI flag) to authorize gate responses? The dashboard is bound to localhost by default, but `--web-bg` users running on shared infrastructure might want explicit token-based auth. +4. **Outer retry classifier (Issue #10).** Is there appetite for a smarter classifier that detects deterministic failures (same parse error twice in a row inside one outer attempt) and skips remaining outer attempts? Saves cost without requiring user configuration. The simpler alternative is a user-facing `retry.max_outer_attempts` knob. --- @@ -608,27 +687,32 @@ For each cluster: 1. **Unit tests** in the relevant `tests/` subdirectory mirroring source layout (per AGENTS.md). 2. **Integration tests** in `tests/test_integration/` that reproduce the actual failure mode from this session, then verify the fix. 3. **Provider parity check** — any change to `providers/copilot.py` mirrored in `providers/claude.py` per [Provider Parity](../../AGENTS.md#provider-parity). -4. **Documentation updates** — `docs/workflow-syntax.md` for Issue #2; `docs/configuration.md` for Issues #4, #6; `CHANGELOG.md` for all clusters. -5. **A real-world smoke test**: re-run the workiq-coach Phase A workflow (with the original `output:` schema on the synthesizer) and verify it now completes. This is the canonical "did we fix the actual problem" test. +4. **Documentation updates** — `docs/workflow-syntax.md` and `docs/configuration.md` for Issue #2 (`output_mode`); `CHANGELOG.md` for all clusters. +5. **Real-world smoke test (the canonical "did we fix it" test):** re-run the workiq-coach Phase A workflow with the original `output:` schema on the synthesizer, against the fixed Conductor. This is Phase 3 of the validation pass that's already documented above. If it now completes first try, Issue #2 is fixed. --- ## References -- Conductor source: `Q:/src/conductor` (this repo, v0.1.16) +- Conductor source: `Q:/src/conductor` (this repo, v0.1.17 as of 2026-05-27) - `AGENTS.md` — architecture overview and contribution guide - `docs/projects/usability-features/` — existing brainstorm/plan documents in this convention - workiq-coach source: `Q:/src/workiq-coach` - `skills/executive-coach-assessor/workflows/phase-a.yaml` — the workflow that surfaced most issues - `skills/executive-coach-assessor/workflows/README.md` — contains the early diagnostic comment about output.py:120 fence-extraction bug -- Conversation context: the analysis was produced collaboratively over a multi-hour debugging session in May 2026. The diagnostic narrative under "Source of evidence" is condensed from that session. +- **Validation cycle (2026-05-27):** + - Phase 1 — static survey, free, ~10 min + - Phase 2 — minimal repro of Issue #1, ~$0.005, ~15 sec runtime + - Phase 3 — end-to-end against original config, ~$0.80, ~50 min runtime (killed mid outer-attempt 2/3) + - Total cost: ~$0.81 +- Conversation context: the original analysis was produced collaboratively over a multi-hour debugging session 2026-05-25/26; the validation pass and this update were produced on 2026-05-27. --- ## What would make this brainstorm into a `.plan.md` -- Maintainer agreement that Cluster A is welcome → opens the door to a PR cluster -- Open question #1 (output mode default) decided → enables a coherent Cluster B -- Anyone disagrees with any of the nine issues → discussion happens here before any code +- **For Issue #2 + #10:** maintainer agreement on Open question #1 (additive `output_mode` field with backward-compat default). Then a `.plan.md` for the implementation. +- **For Issue #3:** trivial enough to skip the `.plan.md` step and just open a PR. +- **For Issue #5 (CLI gate-respond) + #6 (YAML field):** maintainer agreement they're wanted. Then small focused PRs. -If maintainers want any subset of these implemented, the external contributor (Lucio) is happy to open issues + PRs for them. +If maintainers want any subset of these implemented, the external contributor (Lucio) is happy to open issues + PRs for them. The Phase 3 validation evidence is reproducible — happy to provide repro workflows on request. diff --git a/docs/workflow-syntax.md b/docs/workflow-syntax.md index 94f10dca..ea409235 100644 --- a/docs/workflow-syntax.md +++ b/docs/workflow-syntax.md @@ -105,6 +105,14 @@ agents: field_name: type: string description: "Field purpose" + + output_mode: raw # Optional: raw | envelope (default: inferred) + # raw: skip JSON extraction, wrap response + # as {"result": ""}. Cannot be + # combined with output:. + # envelope: explicit opt-in to structured + # output pipeline (same as default when + # output: is declared). tools: # Optional: Agent-specific tools - tool_name @@ -116,6 +124,15 @@ agents: # script, human_gate, workflow). # See docs/configuration.md#reasoning-effort. + retry: # Optional: per-agent retry policy + max_attempts: 3 # 1-10 (default 1 = no retry) + backoff: exponential # exponential | fixed + delay_seconds: 2 # base delay before first retry + retry_on: # error categories that trigger retry + - provider_error + - timeout + max_parse_recovery_attempts: 3 # 0-10; omit for provider default + context_tier: long_context # Optional: per-agent context-tier override # default | long_context (Copilot only) # Overrides runtime.default_context_tier. @@ -129,6 +146,45 @@ agents: when: "{{ condition }}" # Optional: Route condition ``` +### Retry Policy + +Per-agent retry controls how an agent retries on transient failures. The `retry:` block is optional; when omitted the agent makes a single attempt with no retries. + +```yaml +agents: + - name: analyzer + prompt: "Analyze the input" + output: + summary: + type: string + retry: + max_attempts: 3 + backoff: exponential + delay_seconds: 2 + retry_on: + - provider_error + - timeout + max_parse_recovery_attempts: 0 # disable parse recovery for this agent +``` + +| Field | Type | Default | Description | +|-------|------|---------|-------------| +| `max_attempts` | `1-10` | `1` | Total attempts including the first. `1` = no retry. | +| `backoff` | `exponential \| fixed` | `exponential` | Backoff strategy between retries. | +| `delay_seconds` | `0.0-300.0` | `2.0` | Base delay in seconds before the first retry. | +| `retry_on` | list | `[provider_error, timeout]` | Error categories that trigger a retry. | +| `max_parse_recovery_attempts` | `0-10` | Provider default | In-session parse-recovery attempts before giving up. See below. | + +#### `max_parse_recovery_attempts` + +When an agent declares `output:` (structured JSON), the provider tries to parse JSON from the model's response. If parsing fails, a correction prompt is sent in the same session asking the model to fix its response format. This field controls how many correction prompts to send. + +- **Omit** (default): Use the provider default (Copilot=5, Claude=2). +- **`0`**: Disable parse recovery entirely — fail immediately on bad JSON. +- **`1-10`**: Custom limit. + +This is useful when you know an agent's output is simple and a single attempt should suffice, or when you want to fail fast instead of burning tokens on recovery loops. + ### Choosing whether to declare `output:` Declaring `output:` does two things at once: it asks the model to return JSON matching the schema, and it parses the response as structured JSON. For some agents that's what you want. For others it produces parse-recovery loops and burns tokens. @@ -168,6 +224,52 @@ agents: Why this matters: when an `output:` schema is declared, the model is asked to wrap its response in JSON. Large or prose-heavy responses tend to come back inside Markdown code fences, and any triple-backticks in the content can confuse the JSON-extraction step. Omitting `output:` for these agents avoids that whole class of failure and lets the model write naturally. +### `output_mode` + +The `output_mode` field gives you explicit control over how the provider handles the agent's response. It accepts two values: + +| `output_mode` | `output:` declared? | Behavior | +|---|---|---| +| *(not set)* | yes | Default structured-output pipeline: schema injected, JSON parsed and validated | +| *(not set)* | no | Raw response captured as `{"result": ""}` | +| `raw` | no | Same as above, but makes intent explicit — useful for agents that must *never* attempt JSON extraction | +| `raw` | yes | **ValidationError** — these options are incompatible | +| `envelope` | yes | Same as the default structured pipeline (explicit opt-in) | +| `envelope` | no | Raw response captured as `{"result": ""}` | + +**Use `output_mode: raw`** when an agent produces large Markdown reports, code, or free-form prose. This bypasses JSON extraction entirely — no schema instructions are injected, no parse-recovery loop runs, and the model's full response is available as `{{ agent.output.result }}`: + +```yaml +agents: + - name: report_writer + output_mode: raw + prompt: | + Write a detailed analysis report. Include code examples, + tables, and any formatting you need. + # No output: block — output_mode: raw is incompatible with output: + - name: reviewer + prompt: | + Review the following report: + + {{ report_writer.output.result }} +``` + +**Use `output_mode: envelope`** when you want to make the structured-output intent explicit (equivalent to the default when `output:` is declared): + +```yaml +agents: + - name: classifier + output_mode: envelope + prompt: "Classify the input." + output: + category: + type: string + confidence: + type: number +``` + +`output_mode` is only valid on provider-backed agents (the default type). It cannot be set on `script`, `human_gate`, or `workflow` agents. + ### Human Gates Human gates pause workflow execution for user input: diff --git a/src/conductor/cli/app.py b/src/conductor/cli/app.py index 0a5b629f..937aebda 100644 --- a/src/conductor/cli/app.py +++ b/src/conductor/cli/app.py @@ -192,7 +192,7 @@ def _abort_web_bg_if_human_gate(workflow_path: Path, *, skip_gates: bool) -> Non " 1. Use --web (foreground) instead of --web-bg\n" " 2. Add --skip-gates to auto-accept the first option\n" " 3. Remove human_gate steps from the workflow\n" - " 4. Wait for CLI gate-resolution support (planned follow-up)" + " 4. Use `conductor gate-respond --port --choice ` to resolve from CLI" ) typer.echo(message, err=True) raise typer.Exit(code=2) @@ -1256,6 +1256,138 @@ def _print_running_list(entries: list[dict], con: Console) -> None: con.print(table) +@app.command(name="gate-respond") +def gate_respond( + port: Annotated[ + int, + typer.Option( + "--port", + "-p", + help="Dashboard port of the running workflow.", + ), + ], + choice: Annotated[ + str, + typer.Option( + "--choice", + "-c", + help="Selected gate option value.", + ), + ], + agent: Annotated[ + str | None, + typer.Option( + "--agent", + "-a", + help="Gate agent name (auto-discovered via /api/gate-status if omitted).", + ), + ] = None, + input_text: Annotated[ + str | None, + typer.Option( + "--input", + help="Additional input text for the gate response.", + ), + ] = None, + token: Annotated[ + str | None, + typer.Option( + "--token", + help="Auth token (also reads from CONDUCTOR_GATE_TOKEN env var).", + ), + ] = None, +) -> None: + """Resolve a parked human gate from the command line. + + Sends a gate response to a running workflow's web dashboard via HTTP. + Use this when the dashboard UI is unreachable (e.g. SSH session). + + \b + Examples: + conductor gate-respond --port 8080 --choice approve + conductor gate-respond -p 8080 -c reject --agent review-gate + conductor gate-respond -p 8080 -c approve --token secret123 + conductor gate-respond -p 8080 -c approve --input "Looks good" + """ + import httpx + + base_url = f"http://127.0.0.1:{port}" + + # Resolve token from flag or environment variable + resolved_token = token or os.environ.get("CONDUCTOR_GATE_TOKEN") + + # Auto-discover agent name if not provided + if agent is None: + try: + resp = httpx.get(f"{base_url}/api/gate-status", timeout=5) + resp.raise_for_status() + status = resp.json() + if not status.get("waiting"): + console.print(f"[yellow]No gate is currently waiting on port {port}.[/yellow]") + raise typer.Exit(code=1) + agent = status["agent_name"] + except httpx.ConnectError: + console.print( + f"[bold red]Error:[/bold red] Cannot connect to dashboard on port {port}. " + "Is the workflow running with --web or --web-bg?" + ) + raise typer.Exit(code=1) from None + except httpx.HTTPError as exc: + console.print(f"[bold red]Error:[/bold red] Failed to query gate status: {exc}") + raise typer.Exit(code=1) from None + + # Build request body + body: dict[str, Any] = { + "agent_name": agent, + "selected_value": choice, + } + if input_text is not None: + body["additional_input"] = input_text + + # Send the token in the Authorization header (not the body) so it is not + # captured in request-body logs and is compared in constant time server-side. + headers: dict[str, str] = {} + if resolved_token is not None: + headers["Authorization"] = f"Bearer {resolved_token}" + + # Send gate response + try: + resp = httpx.post(f"{base_url}/api/gate-respond", json=body, headers=headers, timeout=10) + except httpx.ConnectError: + console.print( + f"[bold red]Error:[/bold red] Cannot connect to dashboard on port {port}. " + "Is the workflow running with --web or --web-bg?" + ) + raise typer.Exit(code=1) from None + except httpx.HTTPError as exc: + console.print(f"[bold red]Error:[/bold red] Request failed: {exc}") + raise typer.Exit(code=1) from None + + if resp.status_code == 403: + console.print( + "[bold red]Error:[/bold red] Authentication failed. " + "Provide a valid token with --token or CONDUCTOR_GATE_TOKEN env var." + ) + raise typer.Exit(code=1) + if resp.status_code == 409: + detail = resp.json().get("error", "Gate is not waiting for this response") + console.print(f"[bold red]Error:[/bold red] {detail}") + raise typer.Exit(code=1) + if resp.status_code == 422: + detail = resp.json().get("error", "Validation error") + console.print(f"[bold red]Error:[/bold red] {detail}") + raise typer.Exit(code=1) + if resp.status_code != 200: + console.print( + f"[bold red]Error:[/bold red] Unexpected response ({resp.status_code}): {resp.text}" + ) + raise typer.Exit(code=1) + + console.print( + f"[green]Gate resolved:[/green] agent=[cyan]{agent}[/cyan] choice=[cyan]{choice}[/cyan]" + ) + + @app.command() def update( force: bool = typer.Option( diff --git a/src/conductor/config/schema.py b/src/conductor/config/schema.py index 627f391e..1d333e15 100644 --- a/src/conductor/config/schema.py +++ b/src/conductor/config/schema.py @@ -408,6 +408,18 @@ class RetryPolicy(BaseModel): they indicate prompt/schema issues, not transience. """ + max_parse_recovery_attempts: int | None = Field(default=None, ge=0, le=10) + """Maximum in-session parse-recovery attempts before giving up. + + When an agent's response fails JSON extraction, Conductor sends a correction + prompt in the same session. This field controls how many correction prompts + to send. + + - ``None`` (default): Use the provider default (Copilot=5, Claude=2). + - ``0``: Disable parse recovery entirely (fail immediately on bad JSON). + - ``1-10``: Custom limit. + """ + class DialogConfig(BaseModel): """Configuration for agent dialog mode. @@ -638,6 +650,23 @@ class AgentDef(BaseModel): output: dict[str, OutputField] | None = None """Expected output schema for validation.""" + output_mode: Literal["raw", "envelope"] | None = None + """Controls how the provider handles this agent's response. + + - ``raw``: The provider skips schema instruction injection and JSON + extraction entirely. The model's response is wrapped as + ``{"result": ""}``. Incompatible with ``output:`` — if + both are set, validation raises an error. + - ``envelope``: Explicit opt-in to the default structured-output + pipeline. Equivalent to the current behavior when ``output:`` is + declared. + - ``None`` (default): Infer behavior from whether ``output:`` is + declared (backward compatible). + + Only valid on provider-backed agents (type is ``None`` / omitted). + Script, human_gate, and workflow agents cannot set ``output_mode``. + """ + routes: list[RouteDef] = Field(default_factory=list) """Routing rules evaluated in order after execution.""" @@ -1051,6 +1080,8 @@ def validate_agent_type(self) -> AgentDef: raise ValueError( "human_gate agents cannot have 'output_type' (only 'set' agents do)" ) + if self.output_mode is not None: + raise ValueError("human_gate agents cannot have 'output_mode'") elif self.type == "script": if not self.command: raise ValueError("script agents require 'command'") @@ -1095,6 +1126,8 @@ def validate_agent_type(self) -> AgentDef: raise ValueError("script agents cannot have 'values' (only 'set' agents do)") if self.output_type is not None: raise ValueError("script agents cannot have 'output_type' (only 'set' agents do)") + if self.output_mode is not None: + raise ValueError("script agents cannot have 'output_mode'") elif self.type == "workflow": if not self.workflow: raise ValueError("workflow agents require 'workflow' path") @@ -1130,6 +1163,8 @@ def validate_agent_type(self) -> AgentDef: raise ValueError("workflow agents cannot have 'values' (only 'set' agents do)") if self.output_type is not None: raise ValueError("workflow agents cannot have 'output_type' (only 'set' agents do)") + if self.output_mode is not None: + raise ValueError("workflow agents cannot have 'output_mode'") elif self.type == "wait": if self.duration is None: raise ValueError("wait agents require 'duration'") @@ -1187,6 +1222,8 @@ def validate_agent_type(self) -> AgentDef: raise ValueError("wait agents cannot have 'values' (only 'set' agents do)") if self.output_type is not None: raise ValueError("wait agents cannot have 'output_type' (only 'set' agents do)") + if self.output_mode is not None: + raise ValueError("wait agents cannot have 'output_mode'") self._validate_wait_duration() elif self.type == "set": if (self.value is None) == (self.values is None): @@ -1242,6 +1279,8 @@ def validate_agent_type(self) -> AgentDef: raise ValueError("set agents cannot have 'timeout_seconds'") if self.duration is not None: raise ValueError("set agents cannot have 'duration' (only 'wait' agents do)") + if self.output_mode is not None: + raise ValueError("set agents cannot have 'output_mode'") elif self.type == "terminate": # Required fields if self.status is None: @@ -1321,6 +1360,8 @@ def validate_agent_type(self) -> AgentDef: ) if self.duration is not None: raise ValueError("terminate agents cannot have 'duration' (only 'wait' agents do)") + if self.output_mode is not None: + raise ValueError("terminate agents cannot have 'output_mode'") else: # Regular agent or human_gate — input_mapping is not valid if self.input_mapping is not None: @@ -1368,8 +1409,27 @@ def validate_agent_type(self) -> AgentDef: f"'{self.type or 'agent'}' agents cannot have 'reason' " "(only 'terminate' and 'wait' agents support this field)" ) + if self.output_mode == "raw" and self.output: + raise ValueError( + "output_mode 'raw' is incompatible with output schema; " + "remove the output: block or use output_mode: envelope" + ) return self + def effective_output_schema(self) -> dict[str, OutputField] | None: + """Return the structured-output schema providers should enforce, or None. + + Centralizes the rule shared by every provider: an agent has an + effective output schema only when ``output:`` is a non-empty mapping + *and* ``output_mode`` is not ``raw``. An empty ``output: {}`` is + treated as "no schema" so all providers agree (Copilot previously + used a truthiness check while Claude used an ``is not None`` check, + diverging on the empty-dict case). + """ + if self.output and self.output_mode != "raw": + return self.output + return None + def _validate_wait_duration(self) -> None: """Validate ``duration`` for a ``wait`` agent. diff --git a/src/conductor/executor/script.py b/src/conductor/executor/script.py index 9161cbb7..bf0b0e0d 100644 --- a/src/conductor/executor/script.py +++ b/src/conductor/executor/script.py @@ -8,6 +8,8 @@ import asyncio import os +import shutil +import sys from dataclasses import dataclass from typing import TYPE_CHECKING, Any @@ -138,6 +140,23 @@ async def execute( base_env = {**os.environ, "PYTHONUTF8": "1"} env = {**base_env, **agent.env} if agent.env else base_env + # Resolve bare command names and absolute paths against PATH so that a + # bare name (e.g. "python") finds the executable the shell would, and a + # path missing an extension resolves correctly. Resolution uses the + # subprocess's own ``PATH`` (``env`` may override it via ``agent.env``), + # so the resolved binary matches the one the child would have executed. + # Relative paths containing a separator are left untouched so they keep + # resolving against ``working_dir``. Resolution is non-destructive: when + # ``which`` cannot resolve the command we fall back to the rendered value + # and let the FileNotFoundError handler below produce a clear error. + has_separator = os.sep in rendered_command or ( + os.altsep is not None and os.altsep in rendered_command + ) + if os.path.isabs(rendered_command) or not has_separator: + rendered_command = ( + shutil.which(rendered_command, path=env.get("PATH")) or rendered_command + ) + _verbose_log(f" Script: {rendered_command} {' '.join(rendered_args)}") if stdin_payload is not None: _verbose_log(f" Script stdin: {len(stdin_payload)} bytes") @@ -154,10 +173,17 @@ async def execute( env=env, ) except FileNotFoundError as exc: + hint = "" + if sys.platform == "win32": + hint = ( + " Hint: on Windows, include the file extension (e.g. .exe) " + "or use an absolute path." + ) raise ExecutionError( - f"Script '{agent.name}': command not found: '{rendered_command}'", + f"Script '{agent.name}': command not found: '{rendered_command}'" + f" (working_dir={rendered_working_dir or 'cwd'}){hint}", agent_name=agent.name, - suggestion=f"Ensure '{rendered_command}' is installed and in PATH", + suggestion=f"Ensure '{rendered_command}' is installed and on PATH", ) from exc except OSError as e: raise ExecutionError( diff --git a/src/conductor/providers/claude.py b/src/conductor/providers/claude.py index 7b36622c..ccb42826 100644 --- a/src/conductor/providers/claude.py +++ b/src/conductor/providers/claude.py @@ -225,7 +225,6 @@ def __init__( self._sdk_version: str | None = None self._retry_config = retry_config or RetryConfig() self._retry_history: list[dict[str, Any]] = [] # For testing/debugging retries - self._max_parse_recovery_attempts = 2 # Max retry attempts for malformed JSON self._max_schema_depth = 10 # Max nesting depth for recursive schema building self._default_max_agent_iterations = ( max_agent_iterations if max_agent_iterations is not None else 50 @@ -719,7 +718,11 @@ def _resolve_retry_config(self, agent: AgentDef) -> RetryConfig: jitter=self._retry_config.jitter, backoff=retry.backoff, retry_on=list(retry.retry_on), - max_parse_recovery_attempts=self._retry_config.max_parse_recovery_attempts, + max_parse_recovery_attempts=( + retry.max_parse_recovery_attempts + if retry.max_parse_recovery_attempts is not None + else self._retry_config.max_parse_recovery_attempts + ), ) @staticmethod @@ -756,6 +759,13 @@ def _is_retryable_error(self, exception: Exception) -> bool: Returns: True if the error is transient and should be retried. """ + # A ProviderError carries its own retry classification (e.g. parse + # exhaustion is raised with is_retryable=False). Honor it directly + # rather than falling through to SDK-type heuristics that would never + # match it. + if isinstance(exception, ProviderError): + return exception.is_retryable + if anthropic is None: return False @@ -955,9 +965,13 @@ async def _execute_with_retry( # Build tools list: emit_output (for structured output) + MCP tools all_tools: list[dict[str, Any]] = [] - # Add emit_output tool if agent has output schema - if agent.output is not None: - all_tools.extend(self._build_tools_for_structured_output(agent.output)) + # Effective schema check: skip structured output when output_mode is raw + output_schema = agent.effective_output_schema() + has_output_schema = output_schema is not None + + # Add emit_output tool if agent has effective output schema + if output_schema is not None: + all_tools.extend(self._build_tools_for_structured_output(output_schema)) # Append instruction to use the tool messages[-1]["content"] += ( "\n\nPlease use the 'emit_output' tool to return your response " @@ -974,9 +988,6 @@ async def _execute_with_retry( # Use tools if any are defined request_tools: list[dict[str, Any]] | None = all_tools if all_tools else None - # Track if agent has output schema - has_output_schema = agent.output is not None - for attempt in range(1, config.max_attempts + 1): try: # Execute with agentic tool loop @@ -986,13 +997,14 @@ async def _execute_with_retry( temperature=temperature, max_tokens=max_tokens, tools=request_tools, - output_schema=agent.output, + output_schema=output_schema, has_output_schema=has_output_schema, max_iterations=max_agent_iterations, max_session_seconds=max_session_seconds, interrupt_signal=interrupt_signal, event_callback=event_callback, thinking=thinking, + max_parse_recovery_attempts=config.max_parse_recovery_attempts, ) # Handle partial output from mid-agent interrupt @@ -1016,11 +1028,11 @@ async def _execute_with_retry( ) # Extract structured output - content = self._extract_output(response, agent.output) + content = self._extract_output(response, output_schema) # Validate output if schema is defined - if agent.output: - validate_output(content, agent.output) + if output_schema is not None: + validate_output(content, output_schema) # Use total_tokens from the agentic loop (includes all turns) # If available, use it; otherwise fall back to extracting from final response @@ -1341,6 +1353,7 @@ async def _execute_agentic_loop( interrupt_signal: asyncio.Event | None = None, event_callback: EventCallback | None = None, thinking: dict[str, Any] | None = None, + max_parse_recovery_attempts: int | None = None, ) -> tuple[ClaudeResponse, int | None, bool]: """Execute an agentic loop that handles MCP tool calls. @@ -1367,6 +1380,8 @@ async def _execute_agentic_loop( None means no time limit. interrupt_signal: Optional event that signals a mid-agent interrupt. event_callback: Optional callback for streaming SDK events upstream. + max_parse_recovery_attempts: Resolved per-agent parse recovery limit. + None means use the provider-level default. Returns: Tuple of (final_response, total_tokens_used, is_partial). @@ -1441,6 +1456,7 @@ async def _execute_agentic_loop( tools=tools, output_schema=output_schema, thinking=thinking, + max_parse_recovery_attempts=max_parse_recovery_attempts, ) ) else: @@ -1497,6 +1513,7 @@ async def _execute_agentic_loop( tools=tools, output_schema=output_schema, thinking=thinking, + max_parse_recovery_attempts=max_parse_recovery_attempts, ) else: response = await self._execute_api_call( @@ -1776,6 +1793,7 @@ async def _execute_with_parse_recovery( tools: list[dict[str, Any]] | None, output_schema: dict[str, OutputField] | None, thinking: dict[str, Any] | None = None, + max_parse_recovery_attempts: int | None = None, ) -> ClaudeResponse: """Execute API call with parse recovery for malformed JSON responses. @@ -1790,6 +1808,8 @@ async def _execute_with_parse_recovery( max_tokens: Maximum output tokens. tools: Tool definitions for structured output. output_schema: Expected output schema (None if no schema). + max_parse_recovery_attempts: Resolved per-agent parse recovery limit. + None means use the provider-level default. Returns: Claude API response. @@ -1797,6 +1817,11 @@ async def _execute_with_parse_recovery( Raises: ProviderError: If all retry attempts fail with context about attempts. """ + effective_max_recovery = ( + max_parse_recovery_attempts + if max_parse_recovery_attempts is not None + else self._retry_config.max_parse_recovery_attempts + ) # Track recovery attempts for error reporting recovery_history: list[str] = [] @@ -1839,11 +1864,11 @@ async def _execute_with_parse_recovery( recovery_history.append(f"Attempt 0 (initial): {failure_reason}") logger.warning( f"Initial JSON extraction failed: {failure_reason}. " - f"Starting parse recovery (max {self._max_parse_recovery_attempts} attempts)" + f"Starting parse recovery (max {effective_max_recovery} attempts)" ) - for attempt in range(1, self._max_parse_recovery_attempts + 1): - logger.info(f"Parse recovery attempt {attempt}/{self._max_parse_recovery_attempts}") + for attempt in range(1, effective_max_recovery + 1): + logger.info(f"Parse recovery attempt {attempt}/{effective_max_recovery}") # Append recovery message with specific error context recovery_messages = messages.copy() @@ -1904,16 +1929,21 @@ async def _execute_with_parse_recovery( # All recovery attempts exhausted - raise detailed error logger.error( - f"Parse recovery exhausted after {self._max_parse_recovery_attempts} attempts. " + f"Parse recovery exhausted after {effective_max_recovery} attempts. " f"History: {'; '.join(recovery_history)}" ) + # is_retryable=False marks this as terminal: _is_retryable_error() + # honors the flag directly for ProviderError, so the outer retry loop + # will not retry parse exhaustion. raise ProviderError( - f"Failed to extract valid JSON after {self._max_parse_recovery_attempts} " - "recovery attempts", + f"Failed to extract valid JSON after {effective_max_recovery} recovery attempts", suggestion=( "Claude did not use the emit_output tool and returned invalid JSON. " - f"Recovery history: {'; '.join(recovery_history)}" + f"Recovery history: {'; '.join(recovery_history)}. " + "Tip: if this agent produces large or free-form output, " + "add 'output_mode: raw' to skip JSON extraction." ), + is_retryable=False, ) def _diagnose_json_failure(self, text: str) -> str: @@ -2212,14 +2242,15 @@ def _extract_text_content(self, response: Any) -> dict[str, Any]: response: Claude API response. Returns: - Dict with 'text' key containing the response text. + Dict with 'result' key containing the response text. + Uses 'result' (not 'text') to maintain parity with CopilotProvider. """ text_parts = [] for block in response.content: if hasattr(block, "type") and block.type == "text": text_parts.append(block.text) - return {"text": "\n".join(text_parts)} + return {"result": "\n".join(text_parts)} def _extract_structured_output(self, response: Any) -> dict[str, Any] | None: """Extract structured output from tool_use content blocks. diff --git a/src/conductor/providers/copilot.py b/src/conductor/providers/copilot.py index 0edb1416..3f5dcda6 100644 --- a/src/conductor/providers/copilot.py +++ b/src/conductor/providers/copilot.py @@ -505,7 +505,11 @@ def _resolve_retry_config(self, agent: AgentDef) -> RetryConfig: jitter=self._retry_config.jitter, backoff=retry.backoff, retry_on=list(retry.retry_on), - max_parse_recovery_attempts=self._retry_config.max_parse_recovery_attempts, + max_parse_recovery_attempts=( + retry.max_parse_recovery_attempts + if retry.max_parse_recovery_attempts is not None + else self._retry_config.max_parse_recovery_attempts + ), ) async def _execute_with_retry( @@ -548,6 +552,7 @@ async def _execute_with_retry( tools, interrupt_signal=interrupt_signal, event_callback=event_callback, + retry_config=config, ) # Extract usage data from SDK response if available input_tokens = sdk_response.input_tokens if sdk_response else None @@ -685,6 +690,7 @@ async def _execute_sdk_call( tools: list[str] | None = None, interrupt_signal: asyncio.Event | None = None, event_callback: EventCallback | None = None, + retry_config: RetryConfig | None = None, ) -> tuple[dict[str, Any], SDKResponse | None]: """Execute the actual SDK call or mock handler. @@ -695,6 +701,7 @@ async def _execute_sdk_call( tools: List of tool names available to this agent. interrupt_signal: Optional event for mid-agent interrupt signaling. event_callback: Optional callback for streaming SDK events upstream. + retry_config: Resolved per-agent retry config (used for parse recovery limit). Returns: Tuple of (content dict, SDKResponse with usage data or None for mock). @@ -726,8 +733,9 @@ async def _execute_sdk_call( # Build schema description for output schema (used in prompt and recovery) schema_for_prompt: dict[str, Any] | None = None - if agent.output: - schema_for_prompt = self._build_prompt_schema(agent.output) + output_schema = agent.effective_output_schema() + if output_schema is not None: + schema_for_prompt = self._build_prompt_schema(output_schema) schema_desc = json.dumps(schema_for_prompt, indent=2) full_prompt += ( f"\n\n**IMPORTANT: You MUST respond with a JSON object matching this schema:**\n" @@ -889,8 +897,8 @@ async def _execute_sdk_call( cache_read_tokens = sdk_response.cache_read_tokens cache_write_tokens = sdk_response.cache_write_tokens - # If no output schema, we're done - if not agent.output: + # If no output schema (or output_mode is raw), we're done + if output_schema is None: final_usage = SDKResponse( content=response_content, input_tokens=total_input_tokens, @@ -901,7 +909,7 @@ async def _execute_sdk_call( return {"result": response_content}, final_usage # Try to parse the response as JSON with recovery loop - max_recovery = self._retry_config.max_parse_recovery_attempts + max_recovery = (retry_config or self._retry_config).max_parse_recovery_attempts last_parse_error: str | None = None for recovery_attempt in range(max_recovery + 1): # +1 for initial attempt @@ -959,14 +967,16 @@ async def _execute_sdk_call( ) + recovery_response.output_tokens # All recovery attempts exhausted - expected_fields = list(agent.output.keys()) + expected_fields = list(output_schema.keys()) raise ProviderError( f"Failed to parse structured output from agent response: {last_parse_error}", suggestion=( f"Agent was expected to return JSON with fields: {expected_fields}. " - f"Response started with: {response_content[:200]}..." + f"Response started with: {response_content[:500]}... " + "Tip: if this agent produces large or free-form output, " + "add 'output_mode: raw' to skip JSON extraction." ), - is_retryable=True, + is_retryable=False, ) finally: @@ -1350,7 +1360,7 @@ def _extract_json(self, content: str) -> dict[str, Any]: except json.JSONDecodeError: pass - raise ValueError(f"Could not extract JSON from response: {content[:200]}...") + raise ValueError(f"Could not extract JSON from response: {content[:500]}...") def _build_parse_recovery_prompt( self, diff --git a/src/conductor/web/server.py b/src/conductor/web/server.py index 57d2eb08..3fdeb21d 100644 --- a/src/conductor/web/server.py +++ b/src/conductor/web/server.py @@ -20,14 +20,16 @@ import asyncio import contextlib +import hmac import json import logging +import os from collections.abc import AsyncGenerator from contextlib import asynccontextmanager from pathlib import Path, PurePosixPath, PureWindowsPath from typing import Any -from fastapi import FastAPI, WebSocket, WebSocketDisconnect +from fastapi import FastAPI, Request, WebSocket, WebSocketDisconnect from fastapi.responses import FileResponse, JSONResponse from fastapi.staticfiles import StaticFiles @@ -84,6 +86,10 @@ def __init__( # Gate response channel (web client → engine) self._gate_response_queue: asyncio.Queue[dict[str, Any]] = asyncio.Queue() + # Gate waiting state — set/cleared by the engine so the HTTP API + # can report whether a gate is currently waiting for a response. + self._gate_waiting_agent: str | None = None + # Dialog response channel (web client → engine) self._dialog_response_queue: asyncio.Queue[dict[str, Any]] = asyncio.Queue() @@ -240,6 +246,67 @@ async def resume_agent() -> JSONResponse: self._resume_event.set() return JSONResponse({"status": "resuming"}) + @app.get("/api/gate-status") + async def gate_status() -> JSONResponse: + """Return whether a human gate is currently waiting for a response.""" + agent = self._gate_waiting_agent + return JSONResponse({"waiting": agent is not None, "agent_name": agent}) + + @app.post("/api/gate-respond") + async def gate_respond_api(request: Request) -> JSONResponse: + """Resolve a parked human gate via HTTP POST. + + Body: ``{"agent_name": str, "selected_value": str, + "additional_input": str?}`` + + When the ``CONDUCTOR_GATE_TOKEN`` environment variable is set, + the request must carry a matching token in the + ``Authorization: Bearer `` header. + """ + try: + body = await request.json() + except (json.JSONDecodeError, UnicodeDecodeError): + return JSONResponse({"error": "Invalid JSON body"}, status_code=422) + if not isinstance(body, dict): + return JSONResponse( + {"error": "Request body must be a JSON object"}, status_code=422 + ) + + # Validate token if CONDUCTOR_GATE_TOKEN is set. The token is read + # from the Authorization header (not the JSON body) and compared in + # constant time to avoid leaking it via timing or request logs. + if not self._gate_token_ok(request.headers.get("authorization")): + return JSONResponse({"error": "Invalid or missing token"}, status_code=403) + + # Validate required fields + if not body.get("agent_name"): + return JSONResponse( + {"error": "Missing required field: agent_name"}, status_code=422 + ) + if not body.get("selected_value"): + return JSONResponse( + {"error": "Missing required field: selected_value"}, status_code=422 + ) + + # Validate the gate is actually waiting for this agent. Without this + # check a mismatched agent_name would be accepted here (200) and then + # silently discarded by wait_for_gate_response, parking the workflow + # forever while the CLI reports success. + target_error = self._validate_gate_target(body["agent_name"]) + if target_error is not None: + return JSONResponse({"error": target_error}, status_code=409) + + # Put onto gate response queue (same path as WebSocket handler) + self._gate_response_queue.put_nowait( + { + "type": "gate_response", + "agent_name": body["agent_name"], + "selected_value": body["selected_value"], + "additional_input": body.get("additional_input"), + } + ) + return JSONResponse({"status": "accepted"}) + @app.get("/api/files/{file_path:path}") async def get_file(file_path: str) -> JSONResponse: """Serve a local file relative to the workflow root directory. @@ -331,7 +398,21 @@ async def websocket_endpoint(ws: WebSocket) -> None: try: msg = json.loads(raw) if isinstance(msg, dict) and msg.get("type") == "gate_response": - self._gate_response_queue.put_nowait(msg) + # Apply the same auth + waiting-state checks as the + # HTTP endpoint so the WebSocket path cannot bypass + # CONDUCTOR_GATE_TOKEN or resolve a non-waiting gate. + if not self._gate_token_ok(ws.headers.get("authorization")): + logger.warning( + "Rejecting WS gate_response: invalid or missing token" + ) + elif ( + target_error := self._validate_gate_target( + str(msg.get("agent_name", "")) + ) + ) is not None: + logger.warning("Rejecting WS gate_response: %s", target_error) + else: + self._gate_response_queue.put_nowait(msg) elif isinstance(msg, dict) and msg.get("type") in ( "dialog_message", "dialog_decline", @@ -760,11 +841,51 @@ def has_connections(self) -> bool: """ return len(self._connections) > 0 + def _gate_token_ok(self, auth_header: str | None) -> bool: + """Return True if the gate token requirement is satisfied. + + When ``CONDUCTOR_GATE_TOKEN`` is unset, gate responses are always + allowed. Otherwise the header must be ``Authorization: Bearer + `` matching the env var, compared in constant time so the + token cannot be recovered via timing. + + Args: + auth_header: The raw ``Authorization`` header value, or None. + + Returns: + True if the token check passes (or no token is configured). + """ + expected_token = os.environ.get("CONDUCTOR_GATE_TOKEN") + if not expected_token: + return True + scheme, _, presented = (auth_header or "").partition(" ") + return scheme.lower() == "bearer" and hmac.compare_digest(presented, expected_token) + + def _validate_gate_target(self, agent_name: str) -> str | None: + """Validate that a gate response targets the currently-waiting gate. + + Args: + agent_name: The agent name the response is addressed to. + + Returns: + An error message string if no gate is waiting or the name does + not match the waiting gate, otherwise None. + """ + waiting_agent = self._gate_waiting_agent + if waiting_agent is None: + return "No human gate is currently waiting for a response" + if agent_name != waiting_agent: + return ( + f"Gate response targets agent {agent_name!r} but the " + f"waiting gate is {waiting_agent!r}" + ) + return None + async def wait_for_gate_response(self, agent_name: str) -> dict[str, Any]: """Wait for a gate response from a web client. Blocks until a ``gate_response`` message is received via WebSocket - that matches the given agent name. + or HTTP POST that matches the given agent name. Non-matching messages are discarded with a warning. Because conductor only presents one gate at a time, any ``gate_response`` @@ -780,15 +901,31 @@ async def wait_for_gate_response(self, agent_name: str) -> dict[str, Any]: The gate response payload dict with keys ``selected_value`` and optionally ``additional_input``. """ - while True: - msg = await self._gate_response_queue.get() - if msg.get("agent_name") == agent_name: - return msg - logger.warning( - "Discarding stale gate_response for agent %r while waiting on %r", - msg.get("agent_name"), - agent_name, - ) + self._gate_waiting_agent = agent_name + try: + while True: + msg = await self._gate_response_queue.get() + if msg.get("agent_name") == agent_name: + # Drain any responses still queued on resolution. Two + # concurrent submits for this same gate can both pass the + # waiting-state check and enqueue; we consume one here and + # the duplicate would otherwise linger and auto-resolve the + # next same-named gate reached via loop-back. Clearing it now + # (no ``await`` before the queue is empty) prevents that. + while not self._gate_response_queue.empty(): + dup = self._gate_response_queue.get_nowait() + logger.warning( + "Draining duplicate gate_response for agent %r on resolution", + dup.get("agent_name"), + ) + return msg + logger.warning( + "Discarding stale gate_response for agent %r while waiting on %r", + msg.get("agent_name"), + agent_name, + ) + finally: + self._gate_waiting_agent = None async def wait_for_dialog_message(self, agent_name: str, dialog_id: str) -> dict[str, Any]: """Wait for a dialog message or decline from the web client. diff --git a/tests/test_cli/test_gate_respond.py b/tests/test_cli/test_gate_respond.py new file mode 100644 index 00000000..f7f3b7b6 --- /dev/null +++ b/tests/test_cli/test_gate_respond.py @@ -0,0 +1,294 @@ +"""Tests for ``conductor gate-respond`` CLI command. + +Covers: +- Happy path with mock HTTP server +- Unreachable port returns clear error +- Token passed via Authorization header from --token flag +- Token read from CONDUCTOR_GATE_TOKEN env var +- Auto-discovery of agent name via /api/gate-status +- No gate waiting error +- Gate not waiting / agent mismatch (409) error +""" + +from __future__ import annotations + +import json +import os +from unittest.mock import MagicMock, patch + +import httpx +from typer.testing import CliRunner + +from conductor.cli.app import app + +runner = CliRunner() + + +def _mock_response(status_code: int = 200, json_data: dict | None = None) -> MagicMock: + """Create a mock httpx.Response.""" + resp = MagicMock(spec=httpx.Response) + resp.status_code = status_code + resp.text = json.dumps(json_data or {}) + resp.json.return_value = json_data or {} + return resp + + +class TestGateRespondHappyPath: + """Happy path: gate-respond with all required args.""" + + @patch("httpx.post") + def test_basic_resolve(self, mock_post: MagicMock) -> None: + """gate-respond --port 8080 --choice approve --agent review-gate succeeds.""" + mock_post.return_value = _mock_response(200, {"status": "accepted"}) + + result = runner.invoke( + app, ["gate-respond", "--port", "8080", "--choice", "approve", "--agent", "review-gate"] + ) + assert result.exit_code == 0 + assert "Gate resolved" in result.output + + # Verify the POST was called with correct body + mock_post.assert_called_once() + call_kwargs = mock_post.call_args + body = call_kwargs.kwargs.get("json") or call_kwargs[1].get("json") + assert body["agent_name"] == "review-gate" + assert body["selected_value"] == "approve" + + @patch("httpx.post") + def test_with_input_text(self, mock_post: MagicMock) -> None: + """--input flag is forwarded as additional_input.""" + mock_post.return_value = _mock_response(200, {"status": "accepted"}) + + result = runner.invoke( + app, + [ + "gate-respond", + "--port", + "8080", + "--choice", + "approve", + "--agent", + "g1", + "--input", + "LGTM", + ], + ) + assert result.exit_code == 0 + + body = mock_post.call_args.kwargs.get("json") or mock_post.call_args[1]["json"] + assert body["additional_input"] == "LGTM" + + +class TestGateRespondUnreachablePort: + """Unreachable port produces a clear error.""" + + @patch("httpx.post") + def test_connect_error(self, mock_post: MagicMock) -> None: + mock_post.side_effect = httpx.ConnectError("connection refused") + + result = runner.invoke( + app, + ["gate-respond", "--port", "9999", "--choice", "approve", "--agent", "g1"], + ) + assert result.exit_code == 1 + assert "Cannot connect" in result.output + + +class TestGateRespondTokenHandling: + """Token auth via --token flag and CONDUCTOR_GATE_TOKEN env var.""" + + @patch("httpx.post") + def test_token_from_flag(self, mock_post: MagicMock) -> None: + mock_post.return_value = _mock_response(200, {"status": "accepted"}) + + result = runner.invoke( + app, + [ + "gate-respond", + "--port", + "8080", + "--choice", + "approve", + "--agent", + "g1", + "--token", + "my-secret", + ], + ) + assert result.exit_code == 0 + + headers = mock_post.call_args.kwargs.get("headers") or mock_post.call_args[1]["headers"] + assert headers["Authorization"] == "Bearer my-secret" + # Token must NOT be sent in the JSON body. + body = mock_post.call_args.kwargs.get("json") or mock_post.call_args[1]["json"] + assert "token" not in body + + @patch("httpx.post") + def test_token_from_env(self, mock_post: MagicMock) -> None: + mock_post.return_value = _mock_response(200, {"status": "accepted"}) + + with patch.dict(os.environ, {"CONDUCTOR_GATE_TOKEN": "env-token"}): + result = runner.invoke( + app, + ["gate-respond", "--port", "8080", "--choice", "approve", "--agent", "g1"], + ) + assert result.exit_code == 0 + + headers = mock_post.call_args.kwargs.get("headers") or mock_post.call_args[1]["headers"] + assert headers["Authorization"] == "Bearer env-token" + + @patch("httpx.post") + def test_flag_token_overrides_env(self, mock_post: MagicMock) -> None: + mock_post.return_value = _mock_response(200, {"status": "accepted"}) + + with patch.dict(os.environ, {"CONDUCTOR_GATE_TOKEN": "env-token"}): + result = runner.invoke( + app, + [ + "gate-respond", + "--port", + "8080", + "--choice", + "approve", + "--agent", + "g1", + "--token", + "flag-token", + ], + ) + assert result.exit_code == 0 + + headers = mock_post.call_args.kwargs.get("headers") or mock_post.call_args[1]["headers"] + assert headers["Authorization"] == "Bearer flag-token" + + @patch("httpx.post") + def test_no_auth_header_when_no_token(self, mock_post: MagicMock) -> None: + mock_post.return_value = _mock_response(200, {"status": "accepted"}) + + env = {k: v for k, v in os.environ.items() if k != "CONDUCTOR_GATE_TOKEN"} + with patch.dict(os.environ, env, clear=True): + result = runner.invoke( + app, + ["gate-respond", "--port", "8080", "--choice", "approve", "--agent", "g1"], + ) + assert result.exit_code == 0 + + headers = mock_post.call_args.kwargs.get("headers") or mock_post.call_args[1]["headers"] + assert "Authorization" not in headers + + @patch("httpx.post") + def test_403_error_message(self, mock_post: MagicMock) -> None: + mock_post.return_value = _mock_response(403, {"error": "Invalid or missing token"}) + + result = runner.invoke( + app, + ["gate-respond", "--port", "8080", "--choice", "approve", "--agent", "g1"], + ) + assert result.exit_code == 1 + assert "Authentication failed" in result.output + + @patch("httpx.post") + def test_409_error_message(self, mock_post: MagicMock) -> None: + mock_post.return_value = _mock_response( + 409, {"error": "No human gate is currently waiting for a response"} + ) + + result = runner.invoke( + app, + ["gate-respond", "--port", "8080", "--choice", "approve", "--agent", "g1"], + ) + assert result.exit_code == 1 + assert "waiting" in result.output.lower() + + +class TestGateRespondAutoDiscovery: + """Auto-discovery of agent name via /api/gate-status.""" + + @patch("httpx.post") + @patch("httpx.get") + def test_auto_discover_agent(self, mock_get: MagicMock, mock_post: MagicMock) -> None: + mock_get.return_value = _mock_response(200, {"waiting": True, "agent_name": "auto-gate"}) + mock_post.return_value = _mock_response(200, {"status": "accepted"}) + + result = runner.invoke( + app, + ["gate-respond", "--port", "8080", "--choice", "approve"], + ) + assert result.exit_code == 0 + assert "auto-gate" in result.output + + body = mock_post.call_args.kwargs.get("json") or mock_post.call_args[1]["json"] + assert body["agent_name"] == "auto-gate" + + @patch("httpx.get") + def test_no_gate_waiting(self, mock_get: MagicMock) -> None: + mock_get.return_value = _mock_response(200, {"waiting": False, "agent_name": None}) + + result = runner.invoke( + app, + ["gate-respond", "--port", "8080", "--choice", "approve"], + ) + assert result.exit_code == 1 + assert "No gate is currently waiting" in result.output + + @patch("httpx.get") + def test_auto_discover_connect_error(self, mock_get: MagicMock) -> None: + mock_get.side_effect = httpx.ConnectError("refused") + + result = runner.invoke( + app, + ["gate-respond", "--port", "9999", "--choice", "approve"], + ) + assert result.exit_code == 1 + assert "Cannot connect" in result.output + + @patch("httpx.get") + def test_auto_discover_http_error(self, mock_get: MagicMock) -> None: + """A non-connect HTTP error during auto-discovery is reported clearly.""" + mock_get.side_effect = httpx.HTTPError("boom") + + result = runner.invoke( + app, + ["gate-respond", "--port", "8080", "--choice", "approve"], + ) + assert result.exit_code == 1 + assert "Failed to query gate status" in result.output + + +class TestGateRespondErrorResponses: + """Server error responses surface clear messages.""" + + @patch("httpx.post") + def test_post_http_error(self, mock_post: MagicMock) -> None: + """A non-connect HTTP error on the POST is reported clearly.""" + mock_post.side_effect = httpx.HTTPError("boom") + + result = runner.invoke( + app, + ["gate-respond", "--port", "8080", "--choice", "approve", "--agent", "g1"], + ) + assert result.exit_code == 1 + assert "Request failed" in result.output + + @patch("httpx.post") + def test_422_error_message(self, mock_post: MagicMock) -> None: + mock_post.return_value = _mock_response(422, {"error": "selected_value is required"}) + + result = runner.invoke( + app, + ["gate-respond", "--port", "8080", "--choice", "approve", "--agent", "g1"], + ) + assert result.exit_code == 1 + assert "selected_value is required" in result.output + + @patch("httpx.post") + def test_unexpected_status_code(self, mock_post: MagicMock) -> None: + mock_post.return_value = _mock_response(500, {"error": "internal"}) + + result = runner.invoke( + app, + ["gate-respond", "--port", "8080", "--choice", "approve", "--agent", "g1"], + ) + assert result.exit_code == 1 + assert "Unexpected response" in result.output + assert "500" in result.output diff --git a/tests/test_config/test_output_mode.py b/tests/test_config/test_output_mode.py new file mode 100644 index 00000000..1e5ce3b0 --- /dev/null +++ b/tests/test_config/test_output_mode.py @@ -0,0 +1,130 @@ +"""Tests for the output_mode field on AgentDef.""" + +from __future__ import annotations + +import pytest +from pydantic import ValidationError + +from conductor.config.schema import AgentDef, OutputField + + +class TestOutputModeValidation: + """Tests for output_mode validation rules on AgentDef.""" + + def test_raw_without_output_is_valid(self) -> None: + """output_mode='raw' with no output schema is valid.""" + agent = AgentDef(name="a", prompt="p", output_mode="raw") + assert agent.output_mode == "raw" + assert agent.output is None + + def test_envelope_with_output_is_valid(self) -> None: + """output_mode='envelope' with output schema is valid.""" + agent = AgentDef( + name="a", + prompt="p", + output_mode="envelope", + output={"field": OutputField(type="string")}, + ) + assert agent.output_mode == "envelope" + assert agent.output is not None + + def test_raw_with_output_raises_validation_error(self) -> None: + """output_mode='raw' combined with output schema is rejected.""" + with pytest.raises(ValidationError, match="output_mode 'raw' is incompatible"): + AgentDef( + name="a", + prompt="p", + output_mode="raw", + output={"field": OutputField(type="string")}, + ) + + def test_raw_on_script_raises_validation_error(self) -> None: + """output_mode on script agent type is rejected.""" + with pytest.raises(ValidationError, match="script agents cannot have 'output_mode'"): + AgentDef( + name="a", + type="script", + command="echo hi", + output_mode="raw", + ) + + def test_raw_on_human_gate_raises_validation_error(self) -> None: + """output_mode on human_gate agent type is rejected.""" + from conductor.config.schema import GateOption + + with pytest.raises(ValidationError, match="human_gate agents cannot have 'output_mode'"): + AgentDef( + name="a", + type="human_gate", + prompt="Choose", + options=[GateOption(value="yes", label="Yes", route="next")], + output_mode="raw", + ) + + def test_raw_on_workflow_raises_validation_error(self) -> None: + """output_mode on workflow agent type is rejected.""" + with pytest.raises(ValidationError, match="workflow agents cannot have 'output_mode'"): + AgentDef( + name="a", + type="workflow", + workflow="sub.yaml", + output_mode="raw", + ) + + def test_raw_on_wait_raises_validation_error(self) -> None: + """output_mode on wait agent type is rejected.""" + with pytest.raises(ValidationError, match="wait agents cannot have 'output_mode'"): + AgentDef( + name="a", + type="wait", + duration=60, + output_mode="raw", + ) + + def test_raw_on_set_raises_validation_error(self) -> None: + """output_mode on set agent type is rejected.""" + with pytest.raises(ValidationError, match="set agents cannot have 'output_mode'"): + AgentDef( + name="a", + type="set", + value="42", + output_mode="raw", + ) + + def test_raw_on_terminate_raises_validation_error(self) -> None: + """output_mode on terminate agent type is rejected.""" + with pytest.raises(ValidationError, match="terminate agents cannot have 'output_mode'"): + AgentDef( + name="a", + type="terminate", + status="success", + reason="done", + output_mode="raw", + ) + + def test_none_with_output_is_valid(self) -> None: + """output_mode=None (default) with output schema is valid — backward compat.""" + agent = AgentDef( + name="a", + prompt="p", + output={"field": OutputField(type="string")}, + ) + assert agent.output_mode is None + assert agent.output is not None + + def test_none_without_output_is_valid(self) -> None: + """output_mode=None (default) without output schema is valid — backward compat.""" + agent = AgentDef(name="a", prompt="p") + assert agent.output_mode is None + assert agent.output is None + + def test_envelope_without_output_is_valid(self) -> None: + """output_mode='envelope' without output schema is valid (no-op, wraps as result).""" + agent = AgentDef(name="a", prompt="p", output_mode="envelope") + assert agent.output_mode == "envelope" + assert agent.output is None + + def test_invalid_output_mode_value_rejected(self) -> None: + """An invalid output_mode string is rejected by the Literal type.""" + with pytest.raises(ValidationError): + AgentDef(name="a", prompt="p", output_mode="invalid") # type: ignore[arg-type] diff --git a/tests/test_config/test_schema.py b/tests/test_config/test_schema.py index d60f05fb..5e3cc757 100644 --- a/tests/test_config/test_schema.py +++ b/tests/test_config/test_schema.py @@ -1627,6 +1627,45 @@ def test_rejects_invalid_effort(self, effort: object) -> None: RuntimeConfig(default_reasoning_effort=effort) # type: ignore[arg-type] +class TestRetryPolicyMaxParseRecoveryAttempts: + """Tests for RetryPolicy.max_parse_recovery_attempts field.""" + + def test_max_parse_recovery_zero_valid(self) -> None: + """max_parse_recovery_attempts: 0 disables parse recovery.""" + from conductor.config.schema import RetryPolicy + + policy = RetryPolicy(max_parse_recovery_attempts=0) + assert policy.max_parse_recovery_attempts == 0 + + def test_max_parse_recovery_ten_valid(self) -> None: + """max_parse_recovery_attempts: 10 is the upper bound.""" + from conductor.config.schema import RetryPolicy + + policy = RetryPolicy(max_parse_recovery_attempts=10) + assert policy.max_parse_recovery_attempts == 10 + + def test_max_parse_recovery_negative_rejected(self) -> None: + """max_parse_recovery_attempts: -1 is rejected.""" + from conductor.config.schema import RetryPolicy + + with pytest.raises(ValidationError): + RetryPolicy(max_parse_recovery_attempts=-1) + + def test_max_parse_recovery_eleven_rejected(self) -> None: + """max_parse_recovery_attempts: 11 exceeds the upper bound.""" + from conductor.config.schema import RetryPolicy + + with pytest.raises(ValidationError): + RetryPolicy(max_parse_recovery_attempts=11) + + def test_max_parse_recovery_omitted_defaults_to_none(self) -> None: + """Omitting max_parse_recovery_attempts defaults to None (provider default).""" + from conductor.config.schema import RetryPolicy + + policy = RetryPolicy() + assert policy.max_parse_recovery_attempts is None + + class TestAgentDefContextTier: """Tests for the context_tier field on AgentDef.""" diff --git a/tests/test_executor/test_script.py b/tests/test_executor/test_script.py index 8a358190..dd640308 100644 --- a/tests/test_executor/test_script.py +++ b/tests/test_executor/test_script.py @@ -10,6 +10,7 @@ - Working directory - Jinja2 template rendering in command/args - Command not found error +- Command resolution via shutil.which """ from __future__ import annotations @@ -18,6 +19,7 @@ import os import sys import tempfile +from unittest.mock import AsyncMock, patch import pytest @@ -284,6 +286,220 @@ async def test_specific_exit_code(self, executor: ScriptExecutor) -> None: assert output.exit_code == 42 +class TestScriptExecutorCommandResolution: + """Tests for command resolution via ``shutil.which`` in rendered_command. + + Forward-slash paths, missing Windows extensions, and bare command names + are resolved against PATH/PATHEXT. Relative paths containing a separator + are left untouched so they resolve against ``working_dir``. Resolution is + non-destructive: when ``which`` returns ``None`` the rendered command is + used as-is. + """ + + @pytest.mark.asyncio + async def test_bare_name_resolved_via_which(self, executor: ScriptExecutor) -> None: + """A bare command name is resolved to the executable ``which`` finds.""" + agent = AgentDef( + name="test_bare", + type="script", + command="python", + args=["-c", "print('hello')"], + ) + mock_process = AsyncMock() + mock_process.communicate.return_value = (b"hello\n", b"") + mock_process.returncode = 0 + + with ( + patch( + "conductor.executor.script.shutil.which", + return_value="/resolved/bin/python", + ) as mock_which, + patch("asyncio.create_subprocess_exec", return_value=mock_process) as mock_exec, + ): + await executor.execute(agent, {}) + + mock_which.assert_called_once() + assert mock_which.call_args[0][0] == "python" + assert mock_exec.call_args[0][0] == "/resolved/bin/python" + + @pytest.mark.asyncio + async def test_absolute_path_resolved_via_which(self, executor: ScriptExecutor) -> None: + """An absolute path (incl. forward slashes) is resolved via ``which``.""" + agent = AgentDef( + name="test_abs", + type="script", + command="C:/Python314/python", + args=["-c", "print('hello')"], + ) + mock_process = AsyncMock() + mock_process.communicate.return_value = (b"hello\n", b"") + mock_process.returncode = 0 + + with ( + patch( + "conductor.executor.script.shutil.which", + return_value="C:\\Python314\\python.EXE", + ) as mock_which, + patch("conductor.executor.script.os.path.isabs", return_value=True), + patch("asyncio.create_subprocess_exec", return_value=mock_process) as mock_exec, + ): + await executor.execute(agent, {}) + + mock_which.assert_called_once() + assert mock_which.call_args[0][0] == "C:/Python314/python" + assert mock_exec.call_args[0][0] == "C:\\Python314\\python.EXE" + + @pytest.mark.asyncio + async def test_which_none_falls_back_to_rendered(self, executor: ScriptExecutor) -> None: + """When ``which`` cannot resolve, the rendered command is used as-is.""" + agent = AgentDef( + name="test_fallback", + type="script", + command="python", + args=["-c", "print('hello')"], + ) + mock_process = AsyncMock() + mock_process.communicate.return_value = (b"hello\n", b"") + mock_process.returncode = 0 + + with ( + patch("conductor.executor.script.shutil.which", return_value=None), + patch("asyncio.create_subprocess_exec", return_value=mock_process) as mock_exec, + ): + await executor.execute(agent, {}) + + assert mock_exec.call_args[0][0] == "python" + + @pytest.mark.asyncio + async def test_relative_path_with_separator_not_resolved( + self, executor: ScriptExecutor + ) -> None: + """A relative path with a separator is left untouched (working_dir semantics).""" + agent = AgentDef( + name="test_relative", + type="script", + command="./scripts/run.sh", + args=[], + ) + mock_process = AsyncMock() + mock_process.communicate.return_value = (b"", b"") + mock_process.returncode = 0 + + with ( + patch("conductor.executor.script.shutil.which") as mock_which, + patch("conductor.executor.script.os.path.isabs", return_value=False), + patch("asyncio.create_subprocess_exec", return_value=mock_process) as mock_exec, + ): + await executor.execute(agent, {}) + + mock_which.assert_not_called() + assert mock_exec.call_args[0][0] == "./scripts/run.sh" + + @pytest.mark.asyncio + async def test_args_not_resolved(self, executor: ScriptExecutor) -> None: + """Args are never passed through ``which`` (may contain URLs or flags with /).""" + agent = AgentDef( + name="test_args_preserve", + type="script", + command="python", + args=["-c", "print('hello')", "https://example.com/api/v1"], + ) + mock_process = AsyncMock() + mock_process.communicate.return_value = (b"hello\n", b"") + mock_process.returncode = 0 + + with ( + patch( + "conductor.executor.script.shutil.which", return_value="/bin/python" + ) as mock_which, + patch("asyncio.create_subprocess_exec", return_value=mock_process) as mock_exec, + ): + await executor.execute(agent, {}) + + called_args = mock_exec.call_args[0][1:] + assert "https://example.com/api/v1" in called_args + # Only the command itself is resolved — args are never passed through which. + assert mock_which.call_count == 1 + + @pytest.mark.asyncio + async def test_command_resolved_against_agent_env_path(self, executor: ScriptExecutor) -> None: + """``which`` resolves against the subprocess PATH, including agent.env overrides. + + Regression test: previously the command was resolved against the parent + ``os.environ["PATH"]`` before ``env`` was built, so an ``agent.env`` PATH + override silently ran a different binary than the subprocess would use. + """ + agent = AgentDef( + name="test_env_path", + type="script", + command="toolx", + env={"PATH": "/childbin"}, + ) + mock_process = AsyncMock() + mock_process.communicate.return_value = (b"", b"") + mock_process.returncode = 0 + + with ( + patch( + "conductor.executor.script.shutil.which", return_value="/childbin/toolx" + ) as mock_which, + patch("asyncio.create_subprocess_exec", return_value=mock_process) as mock_exec, + ): + await executor.execute(agent, {}) + + # which must be called with the subprocess's PATH (the agent.env override), + # not the parent process PATH. + assert mock_which.call_args.kwargs.get("path") == "/childbin" + assert mock_exec.call_args[0][0] == "/childbin/toolx" + + @pytest.mark.asyncio + async def test_file_not_found_includes_hint_on_windows(self, executor: ScriptExecutor) -> None: + """FileNotFoundError on Windows includes a path-resolution hint.""" + agent = AgentDef( + name="test_hint", + type="script", + command="C:/nonexistent/python.exe", + ) + with ( + patch("conductor.executor.script.sys") as mock_sys, + patch("conductor.executor.script.shutil.which", return_value=None), + patch("conductor.executor.script.os.path.isabs", return_value=True), + patch( + "asyncio.create_subprocess_exec", + side_effect=FileNotFoundError("not found"), + ), + ): + mock_sys.platform = "win32" + with pytest.raises(ExecutionError, match="Hint: on Windows") as exc_info: + await executor.execute(agent, {}) + + error_msg = str(exc_info.value) + assert "C:/nonexistent/python.exe" in error_msg + assert "working_dir=cwd" in error_msg + + @pytest.mark.asyncio + async def test_file_not_found_no_hint_on_linux(self, executor: ScriptExecutor) -> None: + """FileNotFoundError on Linux does not include the Windows hint.""" + agent = AgentDef( + name="test_no_hint", + type="script", + command="/usr/local/bin/nonexistent", + ) + with ( + patch("conductor.executor.script.sys") as mock_sys, + patch("conductor.executor.script.shutil.which", return_value=None), + patch( + "asyncio.create_subprocess_exec", + side_effect=FileNotFoundError("not found"), + ), + ): + mock_sys.platform = "linux" + with pytest.raises(ExecutionError, match="command not found") as exc_info: + await executor.execute(agent, {}) + + assert "Hint" not in str(exc_info.value) + + # Child snippet that echoes whatever it reads from stdin straight to stdout. _ECHO_STDIN = "import sys; sys.stdout.write(sys.stdin.read())" # Child snippet that prints the number of characters it read from stdin. diff --git a/tests/test_integration/test_claude_mcp_tool_filter.py b/tests/test_integration/test_claude_mcp_tool_filter.py index 83729a56..a6398897 100644 --- a/tests/test_integration/test_claude_mcp_tool_filter.py +++ b/tests/test_integration/test_claude_mcp_tool_filter.py @@ -79,8 +79,8 @@ def _make_provider_with_mcp() -> ClaudeProvider: provider._retry_config.base_delay = 1.0 provider._retry_config.max_delay = 30.0 provider._retry_config.jitter = 0.0 + provider._retry_config.max_parse_recovery_attempts = 2 provider._retry_history = [] - provider._max_parse_recovery_attempts = 2 provider._max_schema_depth = 10 provider._default_max_agent_iterations = 50 provider._default_max_session_seconds = None diff --git a/tests/test_providers/test_claude.py b/tests/test_providers/test_claude.py index fd60e211..4ff57445 100644 --- a/tests/test_providers/test_claude.py +++ b/tests/test_providers/test_claude.py @@ -290,7 +290,7 @@ async def test_execute_simple_message( rendered_prompt="Say hello", ) - assert result.content == {"text": "Hello, world!"} + assert result.content == {"result": "Hello, world!"} assert result.tokens_used == 15 assert result.model == "claude-3-5-sonnet-latest" @@ -743,7 +743,7 @@ async def test_extract_text_content_multiple_blocks( ) # Verify both text blocks are combined with newline separator - assert result.content == {"text": "First part. \nSecond part."} + assert result.content == {"result": "First part. \nSecond part."} class TestParseRecovery: @@ -931,7 +931,7 @@ async def test_no_parse_recovery_when_no_output_schema( ) # Should succeed without retries - assert result.content == {"text": "This is just plain text"} + assert result.content == {"result": "This is just plain text"} assert mock_client.messages.create.call_count == 1 @@ -1880,6 +1880,31 @@ def __init__(self, status_code: int) -> None: error_429 = MockAPIStatusError(429) assert provider._is_retryable_error(error_429) is True + @patch("conductor.providers.claude.ANTHROPIC_SDK_AVAILABLE", True) + @patch("conductor.providers.claude.AsyncAnthropic") + @patch("conductor.providers.claude.anthropic") + @pytest.mark.asyncio + async def test_is_retryable_error_honors_provider_error_flag( + self, mock_anthropic_module: Mock, mock_anthropic_class: Mock + ) -> None: + """A ProviderError's own is_retryable flag is honored over SDK heuristics.""" + from conductor.exceptions import ProviderError + + mock_anthropic_module.__version__ = "0.77.0" + mock_client = Mock() + mock_client.models.list = AsyncMock(return_value=Mock(data=[])) + mock_anthropic_class.return_value = mock_client + + provider = ClaudeProvider() + + # Parse-exhaustion is raised with is_retryable=False — must not retry. + non_retryable = ProviderError("Failed to parse output", is_retryable=False) + assert provider._is_retryable_error(non_retryable) is False + + # A ProviderError explicitly marked retryable must retry. + retryable = ProviderError("Connection timeout", is_retryable=True) + assert provider._is_retryable_error(retryable) is True + @patch("conductor.providers.claude.ANTHROPIC_SDK_AVAILABLE", True) @patch("conductor.providers.claude.AsyncAnthropic") @patch("conductor.providers.claude.anthropic") @@ -2081,7 +2106,7 @@ def __init__(self) -> None: result = await provider.execute(agent, {}, "Test prompt") # Verify we got a successful response - assert result.content["text"] == "Success" + assert result.content["result"] == "Success" # Verify retry was attempted assert len(provider._retry_history) == 1 assert provider._retry_history[0]["is_retryable"] is True @@ -2125,7 +2150,7 @@ class MockAPITimeoutError(Exception): result = await provider.execute(agent, {}, "Test prompt") - assert result.content["text"] == "Success" + assert result.content["result"] == "Success" assert len(provider._retry_history) == 1 assert provider._retry_history[0]["is_retryable"] is True diff --git a/tests/test_providers/test_claude_edge_cases.py b/tests/test_providers/test_claude_edge_cases.py index bf4a517a..bf2e306e 100644 --- a/tests/test_providers/test_claude_edge_cases.py +++ b/tests/test_providers/test_claude_edge_cases.py @@ -75,7 +75,7 @@ async def test_empty_response_handling( rendered_prompt = "Test prompt" result = await provider.execute(agent, context, rendered_prompt) - assert result.content == {"text": ""} + assert result.content == {"result": ""} @patch("conductor.providers.claude.ANTHROPIC_SDK_AVAILABLE", True) @patch("conductor.providers.claude.AsyncAnthropic") diff --git a/tests/test_providers/test_claude_event_callback.py b/tests/test_providers/test_claude_event_callback.py index 521afebb..02dd2564 100644 --- a/tests/test_providers/test_claude_event_callback.py +++ b/tests/test_providers/test_claude_event_callback.py @@ -58,8 +58,8 @@ def _make_provider_with_mcp() -> ClaudeProvider: provider._default_max_tokens = 8192 provider._retry_config = MagicMock() provider._retry_config.max_attempts = 1 + provider._retry_config.max_parse_recovery_attempts = 2 provider._retry_history = [] - provider._max_parse_recovery_attempts = 2 provider._max_schema_depth = 10 provider._default_max_agent_iterations = 50 provider._default_max_session_seconds = None @@ -85,8 +85,8 @@ def _make_bare_provider() -> ClaudeProvider: provider._default_max_tokens = 8192 provider._retry_config = MagicMock() provider._retry_config.max_attempts = 1 + provider._retry_config.max_parse_recovery_attempts = 2 provider._retry_history = [] - provider._max_parse_recovery_attempts = 2 provider._max_schema_depth = 10 provider._default_max_agent_iterations = 50 provider._default_max_session_seconds = None @@ -550,6 +550,8 @@ async def test_callback_reaches_agentic_loop(self) -> None: # Create a minimal agent mock agent = MagicMock() agent.output = None + agent.output_mode = "auto" + agent.effective_output_schema.return_value = None agent.model = None agent.max_agent_iterations = None agent.max_session_seconds = None diff --git a/tests/test_providers/test_claude_interrupt.py b/tests/test_providers/test_claude_interrupt.py index 10d5955b..9c9f7a75 100644 --- a/tests/test_providers/test_claude_interrupt.py +++ b/tests/test_providers/test_claude_interrupt.py @@ -36,8 +36,8 @@ def _make_provider() -> ClaudeProvider: provider._retry_config.base_delay = 1.0 provider._retry_config.max_delay = 30.0 provider._retry_config.jitter = 0.0 + provider._retry_config.max_parse_recovery_attempts = 2 provider._retry_history = [] - provider._max_parse_recovery_attempts = 2 provider._max_schema_depth = 10 provider._default_max_agent_iterations = 50 provider._default_max_session_seconds = None diff --git a/tests/test_providers/test_claude_mcp_tool_filter.py b/tests/test_providers/test_claude_mcp_tool_filter.py index bdf12350..e8a45cce 100644 --- a/tests/test_providers/test_claude_mcp_tool_filter.py +++ b/tests/test_providers/test_claude_mcp_tool_filter.py @@ -30,8 +30,8 @@ def _make_provider_with_mcp_tools(tools: list[dict[str, Any]]) -> ClaudeProvider provider._default_max_tokens = 8192 provider._retry_config = MagicMock() provider._retry_config.max_attempts = 1 + provider._retry_config.max_parse_recovery_attempts = 2 provider._retry_history = [] - provider._max_parse_recovery_attempts = 2 provider._max_schema_depth = 10 # Set up a mock MCP manager with tools @@ -169,8 +169,8 @@ def _make_bare_provider() -> ClaudeProvider: provider._default_max_tokens = 8192 provider._retry_config = MagicMock() provider._retry_config.max_attempts = 1 + provider._retry_config.max_parse_recovery_attempts = 2 provider._retry_history = [] - provider._max_parse_recovery_attempts = 2 provider._max_schema_depth = 10 return provider diff --git a/tests/test_providers/test_output_mode.py b/tests/test_providers/test_output_mode.py new file mode 100644 index 00000000..ceaf9fe8 --- /dev/null +++ b/tests/test_providers/test_output_mode.py @@ -0,0 +1,403 @@ +"""Tests for output_mode behavior in Copilot and Claude providers. + +Tests cover: +- E1-T5: output_mode=raw skips schema injection, wraps response as {"result": ...} +- E1-T9: Parse-exhaustion raises ProviderError with is_retryable=False +""" + +from __future__ import annotations + +from typing import Any +from unittest.mock import AsyncMock, Mock, patch + +import pytest + +from conductor.config.schema import AgentDef, OutputField +from conductor.exceptions import ProviderError +from conductor.providers.copilot import CopilotProvider, RetryConfig + +# ── Copilot provider tests ────────────────────────────────────────────── + + +def _make_copilot_handler( + response: dict[str, Any], +) -> Any: + """Create a mock handler that returns a fixed response.""" + + def handler(agent: AgentDef, prompt: str, context: dict[str, Any]) -> dict[str, Any]: + return response + + return handler + + +class TestCopilotOutputModeRaw: + """output_mode=raw with the Copilot provider.""" + + @pytest.mark.asyncio + async def test_raw_agent_wraps_response_as_result(self) -> None: + """output_mode=raw wraps a plain-text SDK response as {"result": }. + + Drives the real SDK path (not the mock_handler short-circuit, which + returns a fixed dict and bypasses the output_mode wrapping logic) so + the wrapping is actually exercised: the model returns plain text that + is *not* JSON, and raw mode must wrap it verbatim instead of trying to + extract a JSON object. + """ + from conductor.providers.copilot import SDKResponse + + provider = CopilotProvider() + provider._started = True + mock_session = AsyncMock() + mock_session.session_id = "test-session" + mock_session.disconnect = AsyncMock() + mock_client = AsyncMock() + mock_client.create_session = AsyncMock(return_value=mock_session) + provider._client = mock_client + + agent = AgentDef(name="a", prompt="p", model="gpt-4", output_mode="raw") + with ( + patch("conductor.providers.copilot.COPILOT_SDK_AVAILABLE", True), + patch.object( + provider, + "_send_and_wait", + AsyncMock(return_value=SDKResponse(content="some raw text")), + ), + ): + result, _ = await provider._execute_sdk_call(agent, "p", {}) + assert result == {"result": "some raw text"} + + @pytest.mark.asyncio + async def test_raw_agent_no_schema_instruction_in_prompt(self) -> None: + """output_mode=raw must not inject schema instructions into the prompt. + + Uses the SDK mock path so the full prompt-building code runs, then + asserts the schema-injection marker is absent. + """ + from conductor.providers.copilot import SDKResponse + + provider = CopilotProvider() + provider._started = True + + # Mock the SDK client and session + mock_session = AsyncMock() + mock_session.session_id = "test-session" + mock_session.disconnect = AsyncMock() + mock_client = AsyncMock() + mock_client.create_session = AsyncMock(return_value=mock_session) + provider._client = mock_client + + # Capture the prompt sent to _send_and_wait + captured_prompts: list[str] = [] + + async def capturing_send(session: Any, prompt: str, *args: Any, **kwargs: Any) -> Any: + captured_prompts.append(prompt) + return SDKResponse(content="raw text") + + agent = AgentDef(name="a", prompt="p", model="gpt-4", output_mode="raw") + + with ( + patch("conductor.providers.copilot.COPILOT_SDK_AVAILABLE", True), + patch.object(provider, "_send_and_wait", AsyncMock(side_effect=capturing_send)), + ): + result, _ = await provider._execute_sdk_call(agent, "p", {}) + + assert len(captured_prompts) == 1 + # The schema-injection marker must NOT be present + assert "IMPORTANT: You MUST respond with a JSON object" not in captured_prompts[0] + assert result == {"result": "raw text"} + + @pytest.mark.asyncio + async def test_envelope_with_output_is_backward_compatible(self) -> None: + """output_mode=envelope with output: schema extracts JSON like the default. + + Drives the real SDK path so the schema-extraction logic runs. The + mock_handler short-circuit would return a fixed dict and never exercise + extraction, making the assertion tautological. + """ + from conductor.providers.copilot import SDKResponse + + provider = CopilotProvider() + provider._started = True + mock_session = AsyncMock() + mock_session.session_id = "test-session" + mock_session.disconnect = AsyncMock() + mock_client = AsyncMock() + mock_client.create_session = AsyncMock(return_value=mock_session) + provider._client = mock_client + + agent = AgentDef( + name="a", + prompt="p", + model="gpt-4", + output_mode="envelope", + output={"field": OutputField(type="string")}, + ) + with ( + patch("conductor.providers.copilot.COPILOT_SDK_AVAILABLE", True), + patch.object( + provider, + "_send_and_wait", + AsyncMock(return_value=SDKResponse(content='{"field": "value"}')), + ), + ): + result, _ = await provider._execute_sdk_call(agent, "p", {}) + assert result == {"field": "value"} + + +class TestCopilotParseExhaustionNotRetryable: + """Parse-exhaustion errors in Copilot must be is_retryable=False.""" + + @pytest.mark.asyncio + async def test_parse_exhaustion_is_not_retryable(self) -> None: + """Parse-recovery exhaustion in _execute_sdk_call raises is_retryable=False. + + Drives through the real parse-recovery loop by mocking the SDK + internals so _extract_json fails on every attempt. + """ + from unittest.mock import AsyncMock, patch + + from conductor.providers.copilot import SDKResponse + + provider = CopilotProvider( + retry_config=RetryConfig(max_parse_recovery_attempts=0), + ) + # Bypass _ensure_client_started + provider._started = True + + # Mock the SDK client and session + mock_session = AsyncMock() + mock_session.session_id = "test-session" + mock_session.disconnect = AsyncMock() + mock_client = AsyncMock() + mock_client.create_session = AsyncMock(return_value=mock_session) + provider._client = mock_client + + non_json = SDKResponse(content="This is not valid JSON at all") + + agent = AgentDef( + name="a", + prompt="p", + model="gpt-4", + output={"field": OutputField(type="string")}, + ) + + with ( + patch("conductor.providers.copilot.COPILOT_SDK_AVAILABLE", True), + patch.object(provider, "_send_and_wait", AsyncMock(return_value=non_json)), + ): + with pytest.raises(ProviderError) as exc_info: + await provider._execute_sdk_call(agent, "p", {}) + + assert exc_info.value.is_retryable is False + assert "output_mode: raw" in (exc_info.value.suggestion or "") + + @pytest.mark.asyncio + async def test_parse_exhaustion_error_includes_500_char_prefix(self) -> None: + """Parse-exhaustion suggestion includes first 500 chars of response.""" + provider = CopilotProvider(mock_handler=_make_copilot_handler({"result": "x"})) + # Test the extract_json ValueError message length + long_content = "x" * 600 + with pytest.raises(ValueError, match=r"x{500}\.\.\."): + provider._extract_json(long_content) + + @pytest.mark.asyncio + async def test_no_outer_retry_on_parse_exhaustion(self) -> None: + """Verify parse-exhaustion (is_retryable=False) short-circuits the outer retry.""" + call_count = 0 + + async def fake_sdk_call( + agent: Any, + rendered_prompt: str, + context: Any, + tools: Any = None, + interrupt_signal: Any = None, + event_callback: Any = None, + retry_config: Any = None, + ) -> Any: + nonlocal call_count + call_count += 1 + raise ProviderError( + "Failed to parse structured output", + is_retryable=False, + ) + + provider = CopilotProvider( + retry_config=RetryConfig(max_attempts=3), + ) + provider._execute_sdk_call = fake_sdk_call # type: ignore[assignment] + + agent = AgentDef(name="a", prompt="p", model="gpt-4") + with pytest.raises(ProviderError) as exc_info: + await provider.execute(agent=agent, context={}, rendered_prompt="p") + + assert exc_info.value.is_retryable is False + assert call_count == 1 # No retries — short-circuited on first attempt + + +# ── Claude provider tests ─────────────────────────────────────────────── + + +def _create_text_block(text: str) -> Mock: + block = Mock() + block.type = "text" + block.text = text + return block + + +def _create_tool_use_block(input_dict: dict) -> Mock: + block = Mock() + block.type = "tool_use" + block.id = "tool_123" + block.name = "emit_output" + block.input = input_dict + return block + + +def _create_response(content_blocks: list, msg_id: str = "msg_1") -> Mock: + response = Mock() + response.id = msg_id + response.content = content_blocks + response.model = "claude-3-5-sonnet-latest" + response.stop_reason = "end_turn" + response.usage = Mock( + input_tokens=10, + output_tokens=20, + cache_creation_input_tokens=0, + cache_read_input_tokens=0, + ) + response.type = "message" + response.role = "assistant" + return response + + +@patch("conductor.providers.claude.ANTHROPIC_SDK_AVAILABLE", True) +@patch("conductor.providers.claude.AsyncAnthropic") +@patch("conductor.providers.claude.anthropic") +class TestClaudeOutputModeRaw: + """output_mode=raw with the Claude provider.""" + + @pytest.mark.asyncio + async def test_raw_agent_wraps_response_as_result( + self, mock_anthropic_module: Mock, mock_anthropic_class: Mock + ) -> None: + """output_mode=raw agent returns text wrapped in {"result": ...}.""" + mock_anthropic_module.__version__ = "0.77.0" + + text_response = _create_response([_create_text_block("raw output")]) + mock_client = Mock() + mock_client.messages = Mock() + mock_client.messages.create = AsyncMock(return_value=text_response) + mock_anthropic_class.return_value = mock_client + + from conductor.providers.claude import ClaudeProvider + + provider = ClaudeProvider(api_key="test-key") + agent = AgentDef(name="a", prompt="p", model="claude-3-5-sonnet-latest", output_mode="raw") + result = await provider.execute(agent=agent, context={}, rendered_prompt="p") + + # Raw mode wraps text response as {"result": "..."} — matches Copilot parity + assert result.content == {"result": "raw output"} + + @pytest.mark.asyncio + async def test_raw_agent_no_emit_output_tool_injected( + self, mock_anthropic_module: Mock, mock_anthropic_class: Mock + ) -> None: + """output_mode=raw must not inject the emit_output tool.""" + mock_anthropic_module.__version__ = "0.77.0" + + text_response = _create_response([_create_text_block("raw output")]) + mock_client = Mock() + mock_client.messages = Mock() + mock_client.messages.create = AsyncMock(return_value=text_response) + mock_anthropic_class.return_value = mock_client + + from conductor.providers.claude import ClaudeProvider + + provider = ClaudeProvider(api_key="test-key") + agent = AgentDef(name="a", prompt="p", model="claude-3-5-sonnet-latest", output_mode="raw") + await provider.execute(agent=agent, context={}, rendered_prompt="p") + + # Verify no emit_output tool in the API call + call_kwargs = mock_client.messages.create.call_args + tools_arg = call_kwargs.kwargs.get("tools") if call_kwargs.kwargs else None + # When output_mode=raw, no tools should be injected (unless MCP tools exist) + assert tools_arg is None or not any( + t.get("name") == "emit_output" for t in (tools_arg or []) + ) + + +@patch("conductor.providers.claude.ANTHROPIC_SDK_AVAILABLE", True) +@patch("conductor.providers.claude.AsyncAnthropic") +@patch("conductor.providers.claude.anthropic") +class TestClaudeParseExhaustionNotRetryable: + """Parse-exhaustion errors in Claude must be is_retryable=False.""" + + @pytest.mark.asyncio + async def test_parse_exhaustion_is_not_retryable( + self, mock_anthropic_module: Mock, mock_anthropic_class: Mock + ) -> None: + """After Claude parse recovery exhausts, ProviderError has is_retryable=False.""" + mock_anthropic_module.__version__ = "0.77.0" + + # Every response is text-only (no emit_output tool use) → triggers recovery + bad_response = _create_response([_create_text_block("I cannot format this as JSON")]) + mock_client = Mock() + mock_client.messages = Mock() + mock_client.messages.create = AsyncMock(return_value=bad_response) + mock_anthropic_class.return_value = mock_client + + from conductor.providers.claude import ClaudeProvider + from conductor.providers.claude import RetryConfig as ClaudeRetryConfig + + provider = ClaudeProvider( + api_key="test-key", + retry_config=ClaudeRetryConfig(max_attempts=1, max_parse_recovery_attempts=1), + ) + agent = AgentDef( + name="a", + prompt="p", + model="claude-3-5-sonnet-latest", + output={"field": OutputField(type="string")}, + ) + + with pytest.raises(ProviderError) as exc_info: + await provider.execute(agent=agent, context={}, rendered_prompt="p") + + assert exc_info.value.is_retryable is False + # The parse-exhaustion error is wrapped by the outer retry handler; + # the output_mode hint appears in the wrapped message string. + assert "output_mode: raw" in str(exc_info.value) + + @pytest.mark.asyncio + async def test_no_outer_retry_on_parse_exhaustion( + self, mock_anthropic_module: Mock, mock_anthropic_class: Mock + ) -> None: + """Verify parse-exhaustion does not trigger outer retry in Claude.""" + mock_anthropic_module.__version__ = "0.77.0" + + bad_response = _create_response([_create_text_block("I cannot format this as JSON")]) + mock_client = Mock() + mock_client.messages = Mock() + mock_client.messages.create = AsyncMock(return_value=bad_response) + mock_anthropic_class.return_value = mock_client + + from conductor.providers.claude import ClaudeProvider + from conductor.providers.claude import RetryConfig as ClaudeRetryConfig + + provider = ClaudeProvider( + api_key="test-key", + retry_config=ClaudeRetryConfig(max_attempts=3, max_parse_recovery_attempts=0), + ) + agent = AgentDef( + name="a", + prompt="p", + model="claude-3-5-sonnet-latest", + output={"field": OutputField(type="string")}, + ) + + with pytest.raises(ProviderError) as exc_info: + await provider.execute(agent=agent, context={}, rendered_prompt="p") + + assert exc_info.value.is_retryable is False + # With is_retryable=False, the outer retry loop should only call once + assert mock_client.messages.create.call_count == 1 diff --git a/tests/test_providers/test_parse_recovery_config.py b/tests/test_providers/test_parse_recovery_config.py new file mode 100644 index 00000000..2de0b8d5 --- /dev/null +++ b/tests/test_providers/test_parse_recovery_config.py @@ -0,0 +1,261 @@ +"""Tests for per-agent max_parse_recovery_attempts configuration. + +Verifies that the YAML retry.max_parse_recovery_attempts field is correctly +threaded through both Copilot and Claude providers, overriding provider +defaults when set. +""" + +from __future__ import annotations + +from typing import Any +from unittest.mock import AsyncMock, Mock, patch + +import pytest + +from conductor.config.schema import AgentDef, OutputField, RetryPolicy +from conductor.exceptions import ProviderError + +# --------------------------------------------------------------------------- +# Copilot provider tests +# --------------------------------------------------------------------------- + + +class TestCopilotParseRecoveryConfig: + """Tests that Copilot provider respects per-agent max_parse_recovery_attempts.""" + + def _make_provider(self, mock_handler: Any = None, retry_config: Any = None) -> Any: + from conductor.providers.copilot import CopilotProvider, RetryConfig + + config = retry_config or RetryConfig() + return CopilotProvider(mock_handler=mock_handler, retry_config=config) + + def test_resolve_retry_config_uses_yaml_value(self) -> None: + """Per-agent retry.max_parse_recovery_attempts overrides provider default.""" + from conductor.providers.copilot import RetryConfig + + provider = self._make_provider( + mock_handler=lambda a, p, c: {"result": "ok"}, + retry_config=RetryConfig(max_parse_recovery_attempts=5), + ) + agent = AgentDef( + name="test", + prompt="test", + retry=RetryPolicy(max_parse_recovery_attempts=2), + ) + resolved = provider._resolve_retry_config(agent) + assert resolved.max_parse_recovery_attempts == 2 + + def test_resolve_retry_config_falls_back_to_provider_default(self) -> None: + """When YAML field is omitted (None), provider default is preserved.""" + from conductor.providers.copilot import RetryConfig + + provider = self._make_provider( + mock_handler=lambda a, p, c: {"result": "ok"}, + retry_config=RetryConfig(max_parse_recovery_attempts=5), + ) + agent = AgentDef( + name="test", + prompt="test", + retry=RetryPolicy(), # max_parse_recovery_attempts=None + ) + resolved = provider._resolve_retry_config(agent) + assert resolved.max_parse_recovery_attempts == 5 + + def test_resolve_retry_config_zero_disables_recovery(self) -> None: + """max_parse_recovery_attempts=0 resolves to 0 (disable recovery).""" + from conductor.providers.copilot import RetryConfig + + provider = self._make_provider( + mock_handler=lambda a, p, c: {"result": "ok"}, + retry_config=RetryConfig(max_parse_recovery_attempts=5), + ) + agent = AgentDef( + name="test", + prompt="test", + retry=RetryPolicy(max_parse_recovery_attempts=0), + ) + resolved = provider._resolve_retry_config(agent) + assert resolved.max_parse_recovery_attempts == 0 + + def test_no_retry_policy_uses_provider_default(self) -> None: + """Agent without retry policy gets provider-level default (5).""" + from conductor.providers.copilot import RetryConfig + + provider = self._make_provider( + mock_handler=lambda a, p, c: {"result": "ok"}, + retry_config=RetryConfig(max_parse_recovery_attempts=5), + ) + agent = AgentDef(name="test", prompt="test") + resolved = provider._resolve_retry_config(agent) + assert resolved.max_parse_recovery_attempts == 5 + + +# --------------------------------------------------------------------------- +# Claude provider tests +# --------------------------------------------------------------------------- + + +class TestClaudeParseRecoveryConfig: + """Tests that Claude provider respects per-agent max_parse_recovery_attempts.""" + + @patch("conductor.providers.claude.ANTHROPIC_SDK_AVAILABLE", True) + @patch("conductor.providers.claude.AsyncAnthropic") + @patch("conductor.providers.claude.anthropic") + def test_resolve_retry_config_uses_yaml_value( + self, mock_anthropic_module: Mock, mock_anthropic_class: Mock + ) -> None: + """Per-agent retry.max_parse_recovery_attempts overrides provider default.""" + mock_anthropic_module.__version__ = "0.77.0" + from conductor.providers.claude import ClaudeProvider, RetryConfig + + provider = ClaudeProvider(retry_config=RetryConfig(max_parse_recovery_attempts=2)) + agent = AgentDef( + name="test", + prompt="test", + retry=RetryPolicy(max_parse_recovery_attempts=7), + ) + resolved = provider._resolve_retry_config(agent) + assert resolved.max_parse_recovery_attempts == 7 + + @patch("conductor.providers.claude.ANTHROPIC_SDK_AVAILABLE", True) + @patch("conductor.providers.claude.AsyncAnthropic") + @patch("conductor.providers.claude.anthropic") + def test_resolve_retry_config_falls_back_to_provider_default( + self, mock_anthropic_module: Mock, mock_anthropic_class: Mock + ) -> None: + """When YAML field is omitted (None), provider default is preserved.""" + mock_anthropic_module.__version__ = "0.77.0" + from conductor.providers.claude import ClaudeProvider, RetryConfig + + provider = ClaudeProvider(retry_config=RetryConfig(max_parse_recovery_attempts=2)) + agent = AgentDef( + name="test", + prompt="test", + retry=RetryPolicy(), # max_parse_recovery_attempts=None + ) + resolved = provider._resolve_retry_config(agent) + assert resolved.max_parse_recovery_attempts == 2 + + @patch("conductor.providers.claude.ANTHROPIC_SDK_AVAILABLE", True) + @patch("conductor.providers.claude.AsyncAnthropic") + @patch("conductor.providers.claude.anthropic") + def test_resolve_retry_config_zero_disables_recovery( + self, mock_anthropic_module: Mock, mock_anthropic_class: Mock + ) -> None: + """max_parse_recovery_attempts=0 resolves to 0 (disable recovery).""" + mock_anthropic_module.__version__ = "0.77.0" + from conductor.providers.claude import ClaudeProvider, RetryConfig + + provider = ClaudeProvider(retry_config=RetryConfig(max_parse_recovery_attempts=2)) + agent = AgentDef( + name="test", + prompt="test", + retry=RetryPolicy(max_parse_recovery_attempts=0), + ) + resolved = provider._resolve_retry_config(agent) + assert resolved.max_parse_recovery_attempts == 0 + + @patch("conductor.providers.claude.ANTHROPIC_SDK_AVAILABLE", True) + @patch("conductor.providers.claude.AsyncAnthropic") + @patch("conductor.providers.claude.anthropic") + def test_no_retry_policy_uses_provider_default( + self, mock_anthropic_module: Mock, mock_anthropic_class: Mock + ) -> None: + """Agent without retry policy gets provider-level default (2).""" + mock_anthropic_module.__version__ = "0.77.0" + from conductor.providers.claude import ClaudeProvider, RetryConfig + + provider = ClaudeProvider(retry_config=RetryConfig(max_parse_recovery_attempts=2)) + agent = AgentDef(name="test", prompt="test") + resolved = provider._resolve_retry_config(agent) + assert resolved.max_parse_recovery_attempts == 2 + + @patch("conductor.providers.claude.ANTHROPIC_SDK_AVAILABLE", True) + @patch("conductor.providers.claude.AsyncAnthropic") + @patch("conductor.providers.claude.anthropic") + @pytest.mark.asyncio + async def test_parse_recovery_zero_skips_recovery_loop( + self, mock_anthropic_module: Mock, mock_anthropic_class: Mock + ) -> None: + """When max_parse_recovery_attempts=0, parse recovery raises immediately.""" + mock_anthropic_module.__version__ = "0.77.0" + + # Create a text-only response (no tool_use, no valid JSON) to trigger recovery + text_block = Mock() + text_block.type = "text" + text_block.text = "This is not JSON at all" + + bad_response = Mock() + bad_response.id = "msg_bad" + bad_response.content = [text_block] + bad_response.model = "claude-3-5-sonnet-latest" + bad_response.stop_reason = "end_turn" + bad_response.usage = Mock(input_tokens=10, output_tokens=20, cache_creation_input_tokens=0) + bad_response.type = "message" + bad_response.role = "assistant" + + mock_client = Mock() + mock_client.messages = Mock() + # Only ONE API call should be made (no recovery calls) + mock_client.messages.create = AsyncMock(return_value=bad_response) + mock_client.models = Mock() + mock_client.models.list = AsyncMock(return_value=Mock(data=[])) + mock_client.close = AsyncMock() + mock_anthropic_class.return_value = mock_client + + from conductor.providers.claude import ClaudeProvider + + provider = ClaudeProvider() + + # Call _execute_with_parse_recovery directly with max_parse_recovery_attempts=0 + with pytest.raises(ProviderError, match="Failed to extract valid JSON after 0"): + await provider._execute_with_parse_recovery( + messages=[{"role": "user", "content": "test"}], + model="claude-3-5-sonnet-latest", + temperature=0.7, + max_tokens=1024, + tools=None, + output_schema={"answer": OutputField(type="string")}, + max_parse_recovery_attempts=0, + ) + + # Exactly 1 API call: the initial attempt, no recovery calls + assert mock_client.messages.create.call_count == 1 + await provider.close() + + +class TestCopilotParseRecoveryThreading: + """Tests that per-agent config is threaded to the parse recovery loop in Copilot.""" + + @pytest.mark.asyncio + async def test_retry_config_threaded_to_sdk_call(self) -> None: + """The resolved retry_config is passed to _execute_sdk_call.""" + from conductor.providers.copilot import CopilotProvider, RetryConfig + + call_args: list[dict[str, Any]] = [] + + async def capture_sdk_call(self: Any, *args: Any, **kwargs: Any) -> Any: + call_args.append(kwargs) + # Return a simple response to avoid further processing + return {"result": "ok"}, None + + provider = CopilotProvider( + mock_handler=lambda a, p, c: {"result": "ok"}, + retry_config=RetryConfig(max_parse_recovery_attempts=5), + ) + + agent = AgentDef( + name="test", + prompt="test", + retry=RetryPolicy(max_parse_recovery_attempts=3), + ) + + with patch.object(CopilotProvider, "_execute_sdk_call", capture_sdk_call): + await provider.execute( + agent=agent, + context={"workflow": {"input": {}}}, + rendered_prompt="test", + ) + + assert len(call_args) == 1 + assert call_args[0]["retry_config"].max_parse_recovery_attempts == 3 diff --git a/tests/test_web/test_gate_respond_api.py b/tests/test_web/test_gate_respond_api.py new file mode 100644 index 00000000..a8b86098 --- /dev/null +++ b/tests/test_web/test_gate_respond_api.py @@ -0,0 +1,347 @@ +"""Tests for POST /api/gate-respond and GET /api/gate-status endpoints. + +Covers: +- Valid gate-respond request returns 200 and payload lands on queue +- Missing selected_value returns 422 +- agent_name not matching the waiting gate returns 409 +- no gate waiting returns 409 +- Token mismatch when CONDUCTOR_GATE_TOKEN is set returns 403 (Authorization header) +- No token required when env var is unset +- Gate-status returns waiting state correctly +""" + +from __future__ import annotations + +import asyncio +import os +from unittest.mock import patch + +import httpx +from starlette.testclient import TestClient + +from conductor.events import WorkflowEventEmitter +from conductor.web.server import WebDashboard + + +def _make_dashboard() -> tuple[WorkflowEventEmitter, WebDashboard]: + """Create an emitter and dashboard pair for testing.""" + emitter = WorkflowEventEmitter() + dashboard = WebDashboard(emitter, host="127.0.0.1", port=0) + return emitter, dashboard + + +class TestGateRespondValidRequest: + """POST /api/gate-respond with a valid body returns 200 and queues payload.""" + + def test_valid_request_accepted(self) -> None: + _, dashboard = _make_dashboard() + dashboard._gate_waiting_agent = "review-gate" + with TestClient(dashboard.app) as client: + resp = client.post( + "/api/gate-respond", + json={ + "agent_name": "review-gate", + "selected_value": "approve", + }, + ) + assert resp.status_code == 200 + assert resp.json() == {"status": "accepted"} + + # Verify payload landed on the queue + msg = dashboard._gate_response_queue.get_nowait() + assert msg["type"] == "gate_response" + assert msg["agent_name"] == "review-gate" + assert msg["selected_value"] == "approve" + + def test_valid_request_with_additional_input(self) -> None: + _, dashboard = _make_dashboard() + dashboard._gate_waiting_agent = "review-gate" + with TestClient(dashboard.app) as client: + resp = client.post( + "/api/gate-respond", + json={ + "agent_name": "review-gate", + "selected_value": "approve", + "additional_input": "Looks good to me", + }, + ) + assert resp.status_code == 200 + + msg = dashboard._gate_response_queue.get_nowait() + assert msg["additional_input"] == "Looks good to me" + + +class TestGateRespondMissingFields: + """POST /api/gate-respond with missing required fields returns 422.""" + + def test_missing_selected_value(self) -> None: + _, dashboard = _make_dashboard() + with TestClient(dashboard.app) as client: + resp = client.post( + "/api/gate-respond", + json={"agent_name": "review-gate"}, + ) + assert resp.status_code == 422 + assert "selected_value" in resp.json()["error"] + + def test_missing_agent_name(self) -> None: + _, dashboard = _make_dashboard() + with TestClient(dashboard.app) as client: + resp = client.post( + "/api/gate-respond", + json={"selected_value": "approve"}, + ) + assert resp.status_code == 422 + assert "agent_name" in resp.json()["error"] + + +class TestGateRespondMalformedBody: + """POST /api/gate-respond with malformed or non-dict JSON body returns 422.""" + + def test_invalid_json_body(self) -> None: + _, dashboard = _make_dashboard() + with TestClient(dashboard.app) as client: + resp = client.post( + "/api/gate-respond", + content="not json", + headers={"content-type": "application/json"}, + ) + assert resp.status_code == 422 + assert "Invalid JSON" in resp.json()["error"] + + def test_non_dict_json_body(self) -> None: + _, dashboard = _make_dashboard() + with TestClient(dashboard.app) as client: + resp = client.post( + "/api/gate-respond", + content='["a", "b"]', + headers={"content-type": "application/json"}, + ) + assert resp.status_code == 422 + assert "JSON object" in resp.json()["error"] + + def test_null_json_body(self) -> None: + _, dashboard = _make_dashboard() + with TestClient(dashboard.app) as client: + resp = client.post( + "/api/gate-respond", + content="null", + headers={"content-type": "application/json"}, + ) + assert resp.status_code == 422 + assert "JSON object" in resp.json()["error"] + + +class TestGateRespondTokenAuth: + """Token authentication for POST /api/gate-respond.""" + + def test_token_mismatch_returns_403(self) -> None: + _, dashboard = _make_dashboard() + with ( + patch.dict(os.environ, {"CONDUCTOR_GATE_TOKEN": "correct-token"}), + TestClient(dashboard.app) as client, + ): + resp = client.post( + "/api/gate-respond", + json={ + "agent_name": "review-gate", + "selected_value": "approve", + }, + headers={"Authorization": "Bearer wrong-token"}, + ) + assert resp.status_code == 403 + assert "token" in resp.json()["error"].lower() + + def test_missing_token_returns_403_when_required(self) -> None: + _, dashboard = _make_dashboard() + with ( + patch.dict(os.environ, {"CONDUCTOR_GATE_TOKEN": "correct-token"}), + TestClient(dashboard.app) as client, + ): + resp = client.post( + "/api/gate-respond", + json={ + "agent_name": "review-gate", + "selected_value": "approve", + }, + ) + assert resp.status_code == 403 + + def test_token_in_body_is_rejected(self) -> None: + """A token supplied in the JSON body (old behavior) no longer authenticates.""" + _, dashboard = _make_dashboard() + with ( + patch.dict(os.environ, {"CONDUCTOR_GATE_TOKEN": "correct-token"}), + TestClient(dashboard.app) as client, + ): + resp = client.post( + "/api/gate-respond", + json={ + "agent_name": "review-gate", + "selected_value": "approve", + "token": "correct-token", + }, + ) + assert resp.status_code == 403 + + def test_correct_token_accepted(self) -> None: + _, dashboard = _make_dashboard() + dashboard._gate_waiting_agent = "review-gate" + with ( + patch.dict(os.environ, {"CONDUCTOR_GATE_TOKEN": "correct-token"}), + TestClient(dashboard.app) as client, + ): + resp = client.post( + "/api/gate-respond", + json={ + "agent_name": "review-gate", + "selected_value": "approve", + }, + headers={"Authorization": "Bearer correct-token"}, + ) + assert resp.status_code == 200 + + def test_no_token_required_when_env_unset(self) -> None: + _, dashboard = _make_dashboard() + dashboard._gate_waiting_agent = "review-gate" + env = {k: v for k, v in os.environ.items() if k != "CONDUCTOR_GATE_TOKEN"} + with ( + patch.dict(os.environ, env, clear=True), + TestClient(dashboard.app) as client, + ): + resp = client.post( + "/api/gate-respond", + json={ + "agent_name": "review-gate", + "selected_value": "approve", + }, + ) + assert resp.status_code == 200 + + +class TestGateRespondAgentMatch: + """POST /api/gate-respond validates the agent_name against the waiting gate.""" + + def test_no_gate_waiting_returns_409(self) -> None: + _, dashboard = _make_dashboard() + # _gate_waiting_agent defaults to None (no gate parked) + with TestClient(dashboard.app) as client: + resp = client.post( + "/api/gate-respond", + json={"agent_name": "review-gate", "selected_value": "approve"}, + ) + assert resp.status_code == 409 + assert "waiting" in resp.json()["error"].lower() + assert dashboard._gate_response_queue.empty() + + def test_mismatched_agent_returns_409(self) -> None: + _, dashboard = _make_dashboard() + dashboard._gate_waiting_agent = "review-gate" + with TestClient(dashboard.app) as client: + resp = client.post( + "/api/gate-respond", + json={"agent_name": "other-gate", "selected_value": "approve"}, + ) + assert resp.status_code == 409 + error = resp.json()["error"] + assert "other-gate" in error + assert "review-gate" in error + # The mismatched response must NOT be queued. + assert dashboard._gate_response_queue.empty() + + +class TestGateStatus: + """GET /api/gate-status endpoint.""" + + def test_no_gate_waiting(self) -> None: + _, dashboard = _make_dashboard() + with TestClient(dashboard.app) as client: + resp = client.get("/api/gate-status") + assert resp.status_code == 200 + data = resp.json() + assert data["waiting"] is False + assert data["agent_name"] is None + + def test_gate_waiting(self) -> None: + _, dashboard = _make_dashboard() + # Simulate the engine setting the gate waiting state + dashboard._gate_waiting_agent = "review-gate" + with TestClient(dashboard.app) as client: + resp = client.get("/api/gate-status") + assert resp.status_code == 200 + data = resp.json() + assert data["waiting"] is True + assert data["agent_name"] == "review-gate" + + def test_gate_cleared_after_response(self) -> None: + """wait_for_gate_response clears _gate_waiting_agent on return.""" + _, dashboard = _make_dashboard() + + async def _test() -> None: + # Pre-queue a matching response + dashboard._gate_response_queue.put_nowait({"agent_name": "g1", "selected_value": "ok"}) + result = await dashboard.wait_for_gate_response("g1") + assert result["selected_value"] == "ok" + assert dashboard._gate_waiting_agent is None + + asyncio.run(_test()) + + +class TestGateRespondAuthOrdering: + """The token check must take precedence over field validation (security first).""" + + def test_token_check_precedes_field_validation(self) -> None: + """A request missing both the token and required fields returns 403, not 422. + + Pins the ordering as a security property: an unauthenticated caller must + not be able to probe field-validation behavior (or learn which fields are + required) before passing auth. + """ + _, dashboard = _make_dashboard() + with ( + patch.dict(os.environ, {"CONDUCTOR_GATE_TOKEN": "correct-token"}), + TestClient(dashboard.app) as client, + ): + # Empty body: a valid JSON object but missing agent_name/selected_value + # and carrying no Authorization header. + resp = client.post("/api/gate-respond", json={}) + assert resp.status_code == 403 + assert "token" in resp.json()["error"].lower() + + +class TestGateRespondEndToEnd: + """Full HTTP submit -> engine consumes -> awaiting coroutine continues.""" + + async def test_http_gate_round_trip(self) -> None: + """A POST resolves a parked gate and the awaiting coroutine returns it. + + Exercises the real HTTP endpoint (not a manual queue insert) against a + gate that is genuinely parked in ``wait_for_gate_response``, and asserts + the waiting state is set while parked and cleared after resolution. + """ + _, dashboard = _make_dashboard() + + # Engine side: park a gate. wait_for_gate_response sets _gate_waiting_agent. + wait_task = asyncio.create_task(dashboard.wait_for_gate_response("review-gate")) + await asyncio.sleep(0) # let the task run up to the queue await + assert dashboard._gate_waiting_agent == "review-gate" + + # Client side: resolve it through the real ASGI endpoint in this loop. + transport = httpx.ASGITransport(app=dashboard.app) + async with httpx.AsyncClient(transport=transport, base_url="http://test") as client: + resp = await client.post( + "/api/gate-respond", + json={ + "agent_name": "review-gate", + "selected_value": "approve", + "additional_input": "lgtm", + }, + ) + assert resp.status_code == 200 + + # Engine side: the awaiting coroutine receives the posted response. + result = await asyncio.wait_for(wait_task, timeout=1.0) + assert result["selected_value"] == "approve" + assert result["additional_input"] == "lgtm" + # Waiting state is cleared once the gate resolves. + assert dashboard._gate_waiting_agent is None