feat: add deterministic type: mcp workflow step - #518
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #518 +/- ##
=======================================
Coverage ? 92.00%
=======================================
Files ? 165
Lines ? 27335
Branches ? 0
=======================================
Hits ? 25150
Misses ? 2185
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Six blocking issues here, mostly clustered around the same theme: the new MCP step type leaks more than it should (server-controlled data into trusted metadata, saved results into replay, raw errors past the debug-log boundary that doesn't actually exist for the CLI), plus a real cancellation hole and a namespace collision in structured output. This isn't close to mergeable as-is. The recommended items are mostly follow-up gaps in interrupt handling, pool accounting, validation parity, and docs that promise more than the implementation delivers.
Blocking findings, by location:
engine/workflow.py:806— eviction cleanup swallows cancellation, letting a new tool call start after the workflow was cancelledexecutor/mcp_step.py:153—outputs/errorskeys in a structured result collide with context's own output-shape keysweb/server.py:1010— replaying a checkpoint without the original JSONL synthesizes full MCP results into parallel/for-each groups instead of metadata-onlyengine/workflow.py:1867— a server can put arbitrary data inspill_pathand it gets forwarded as trusted event metadataengine/workflow.py:1840— failures point users at debug logs that--log-filenever actually populatesengine/workflow.py:752—connect_server()'s own exception handler logs raw server output at ERROR, bypassing the sanitization the rest of the step does
In addition, it looks like some merge conflicts crept in.
| evicted_key = target | ||
| evicted = self._mcp_step_managers.pop(target) | ||
| try: | ||
| await evicted.close() |
There was a problem hiding this comment.
BLOCKING: MCPManager.close() swallows CancelledError while it drains connection teardown. Awaiting it directly during eviction means that cancellation gets absorbed here instead of propagating — once cleanup finishes, _get_mcp_step_manager() just carries on connecting and invoking the next tool. So a workflow cancellation, or a fail-fast sibling failure, can still result in a new external side effect starting after the fact.
Run the eviction cleanup in its own shielded task. If cancellation arrives while it's running, keep draining cleanup (tolerating repeated cancellation), then re-raise CancelledError before this code goes on to connect or invoke anything.
There was a problem hiding this comment.
Right — close() absorbs CancelledError by design (task-affine teardown), so awaiting it directly during eviction swallowed the workflow's cancellation and the code went on to connect and invoke.
Fixed in ec64c8a: the eviction close now runs as its own shielded task (_close_evicted_mcp_step_manager), tolerating repeated cancellation while teardown drains, then re-raises CancelledError before any new connect or invoke. Covered by test_eviction_close_reraises_cancellation in tests/test_engine/test_mcp_step_pool.py, which verifies the close completes, the cancellation propagates, and no new connect happens.
| agent.name, | ||
| ", ".join(sorted(shadowed)), | ||
| ) | ||
| envelope.update( |
There was a problem hiding this comment.
BLOCKING: A structured response like {"outputs": [1, 2], "errors": []} gets flattened straight into the envelope, but WorkflowContext and for-each source resolution use those exact two keys to identify group outputs. That means this result gets misclassified and loses its normal .output wrapper — something like {{ call.output.is_error }} breaks even though the tool call succeeded. Affects accumulate, last_only, and explicit context modes alike.
Engine-owned step metadata should decide what counts as a group output, not the payload's own keys — that includes for-each source resolution. Alternatively, reserve outputs/errors from flattening and keep them under structured, documenting the restriction.
There was a problem hiding this comment.
Correct — WorkflowContext duck-types group outputs by exactly those two keys, so a flattened outputs/errors pair misclassified the step's output in every context mode.
Fixed in ec64c8a: outputs and errors are now reserved from flattening alongside content/structured/is_error (see _RESERVED_ENVELOPE_KEYS); they stay reachable under output.structured.outputs / output.structured.errors, and the restriction is documented in the merge-rule sections of docs/workflow-syntax.md and the bundled skill references. Tests: test_outputs_errors_keys_are_reserved_from_flattening plus a context-level regression (test_envelope_with_structured_outputs_keys_keeps_output_wrapper) proving {{ step.output.is_error }} keeps working.
| } | ||
| return "set_started", started_data, "set_completed", completed_data | ||
|
|
||
| if agent_type == "mcp": |
There was a problem hiding this comment.
BLOCKING: The metadata-only synthetic replay path handles standalone MCP steps, but replay_synthetic_from_context() routes saved parallel and for-each entries through _synth_parallel()/_synth_for_each() instead, and those publish the complete saved output — MCP content and structured values included. So resuming without the original JSONL log exposes results in dashboard history and /api/state that live execution deliberately never showed.
Group synthesis needs to know its members' step types, strip MCP envelopes from the aggregate replay events, and synthesize metadata-only member/item events instead. Needs to cover both mixed parallel groups and MCP for-each groups.
There was a problem hiding this comment.
Agreed — live parallel_completed/for_each_completed carry counts only, while the synthetic path published the full saved outputs, MCP envelopes included.
Fixed in ec64c8a: group synthesis now knows member step types. MCP members of a parallel group are stripped from the aggregate and replayed as the metadata-only sequence the live engine emits (mcp_started/mcp_completed with group_name, plus the LLM-less parallel_agent_completed); MCP for-each items get for_each_item_started → mcp_started → mcp_completed → for_each_item_completed (no output), with the envelopes stripped from the aggregate. Mixed groups keep non-MCP members' outputs untouched. Both live and replay events now read truncation markers through the same typed helper. Tests in TestSyntheticReplayMcpGroups (tests/test_web/test_server.py) assert no result value appears anywhere in the emitted events.
| truncated = any(isinstance(block, dict) and block.get("truncated") for block in blocks) | ||
| spill_path = next( | ||
| ( | ||
| block.get("spill_path") |
There was a problem hiding this comment.
BLOCKING: call_tool_structured() preserves content-block extension fields, and this code treats any block's spill_path as if it were metadata Conductor generated itself. A server can return spill_path={"private_result": "value"} and that object gets copied straight into mcp_completed, even with spilling disabled — which both leaks result values through what's supposed to be a metadata-only event and violates the frontend's string type for that field. Synthetic replay repeats the same extraction, and server-supplied truncation fields get the same blind trust.
Keep locally generated truncation/spill metadata separate from whatever the server sent back, or strip those reserved field names before adding local metadata. Live and replay events should build the same trusted, type-checked representation.
There was a problem hiding this comment.
Right — model_dump(mode="json") preserves server extension fields, so a forged spill_path was forwarded as trusted metadata (and could be a non-string, breaking the frontend contract).
Fixed in ec64c8a at ingestion: call_tool_structured strips truncated/spill_path from every server content block, so only Conductor's own truncation pass can set them. The read-side is unified behind mcp_truncation_metadata() (shared by _run_mcp_step and the synthetic replay), which additionally type-checks spill_path as a string — so even envelopes persisted before this change replay safely. Tests: test_server_supplied_truncation_fields_are_stripped (manager) and test_forged_spill_path_in_stored_envelope_is_dropped (replay).
| # base exception — the no-values policy outranks base-exception | ||
| # classification for this step type, and the engine converts real | ||
| # keyboard interrupts before this point. | ||
| logger.debug("MCP step '%s' failed", agent.name, exc_info=True) |
There was a problem hiding this comment.
BLOCKING: Transport, rendering, timeout, and output-validation failures all get replaced with "see debug logs for details," and the actual exception only goes to logger.debug(). But --log-file wires up a Rich console, not a Python logging handler, and doesn't turn DEBUG on. So even someone who explicitly asked for a log file gets nothing to diagnose the failure with.
Add an explicit private diagnostic sink for both run and resume before the exception details get discarded. Keep raw values out of lifecycle events, but preserve safe categories like timeout (and its duration), and have the user-facing message actually point at wherever the diagnostic ends up.
There was a problem hiding this comment.
Accurate — --log-file wires a Rich console, not a logging handler, and nothing turns DEBUG on, so the details were discarded into the void.
Fixed in ec64c8a with an explicit private sink: the engine writes the full exception (including the connect-time cause chain) to a per-run *.mcp-diagnostics.log next to the run's *.events.jsonl, on both run and resume (both wire the event log path into the engine). The redacted message now points at the concrete file (MCP step 'x' failed; full diagnostic: <path>). Safe categories keep authored detail: a per-call timeout surfaces as McpStepTimeoutError with its duration, and the name-only runtime checks propagate verbatim. The workflow-syntax secrets section documents the file's sensitivity. Tests assert both directions: canary never in events/raised errors, and present in the diagnostic file (test_failure_event_is_redacted, test_schema_mismatch_fails_workflow_with_single_redacted_event).
| ...i, | ||
| status: 'completed', | ||
| elapsed: data.elapsed, | ||
| mcp_server: data.server, |
There was a problem hiding this comment.
RECOMMENDED: This handler records MCP metadata on each item, but neither GroupDetail nor ForEachItemRow renders those fields or factors them into hasDetails. Since MCP item completion intentionally skips output, a metadata-only completed item can end up with a disabled detail toggle — no way to see its tool-reported error, server/tool name, result size, truncation, or spill path.
Include the MCP metadata in item expandability and render it in the item detail panel. Keep is_error: true items showing as completed but visibly flag the tool error, without exposing the raw result body.
There was a problem hiding this comment.
Fixed in 79d09fd: ForEachItemRow now treats mcp_server != null as having details (so a metadata-only completed item expands), renders Server/Tool/Result Bytes (with the truncation marker)/Spill Path in the item's metadata grid, and shows a visible Tool reported an error (is_error) warning when mcp_is_error is true while keeping the item's completed status. No raw result body is rendered — none exists on the item by design.
| async def item_call(_server: str, _tool: str, arguments: dict[str, Any]) -> dict[str, Any]: | ||
| if arguments["q"] == "a": | ||
| # Fail only once the sibling is genuinely blocked inside its call. | ||
| await asyncio.wait_for(blocking.wait(), timeout=5) |
There was a problem hiding this comment.
RECOMMENDED: Item a holds the single-server slot while waiting on an event that item b only sets after it acquires that same slot — so the wait times out before the canary-bearing RuntimeError ever gets raised. Accepting any ExecutionError and just asserting the canary is absent doesn't actually establish the exception-redaction behavior this test is meant to cover.
Coordinate the sibling at the slot boundary rather than requiring simultaneous calls into one server. Assert the intended RuntimeError actually happened, that mcp_failed reports that type, and that sibling teardown happens before the pool closes.
There was a problem hiding this comment.
The old coordination was indeed impossible: one shared slot meant item b's call could only start after item a's call finished, so the wait hit its 5s deadline and the canary RuntimeError never fired — the redaction assertions were vacuous.
Rewritten in ec64c8a with roles following call order instead of item identity: the first item to acquire the slot fails immediately with the canary RuntimeError; the sibling is cancelled wherever the fail-fast drain finds it (blocked at the slot boundary or inside its call — both are asserted via the order log). The test now asserts the intended RuntimeError actually happened (mcp_failed reports error_type == "RuntimeError"), the canary appears in no event, and sibling teardown lands before the pool close (["call-raised", "sibling-blocked", "sibling-cancelled", "pool-close"]).
|
|
||
| ### Argument Rendering and Type Coercion | ||
|
|
||
| Dict and list structures in `arguments:` are traversed recursively. String leaves are Jinja2-rendered against workflow context and auto-coerced (e.g. `"105"` -> `105`, `"true"` -> `True`, `"null"` -> `None`). Non-scalar values and embedded templates remain strings. YAML-native scalars (integers, floats, booleans, `None`) pass through untouched. |
There was a problem hiding this comment.
RECOMMENDED: This says non-scalar values and embedded templates stay strings, but _coerce_auto() YAML-parses every fully rendered string. '[1, 2]' becomes a list, '1{{ x }}' becomes the integer 12 when x=2, and 'label: {{ x }}' becomes a mapping. Anyone following this doc as written can end up sending arguments with a type they didn't expect.
Describe the actual behavior: whole-render YAML parsing, including collection conversion and embedded templates whose rendered text parses as something else. The syntax guide and executor docstrings should say the same thing.
There was a problem hiding this comment.
Correct — the description didn't match _coerce_auto, which YAML-parses the whole rendered string. Rewritten in ec64c8a (authoring.md, the executor docstrings, and docs/workflow-syntax.md now agree): "[1, 2]" becomes a list, "1{{ x }}" with x=2 renders "12" and becomes the integer 12, "label: {{ x }}" becomes a mapping, and only renders that parse as plain strings stay strings. Both docs now advise keeping rendered text unambiguous when the exact type matters.
| **Validation rules:** | ||
|
|
||
| - **Static validation (`conductor validate`):** Validates workflows offline without connecting to servers. Checks that the referenced server is declared in `runtime.mcp_servers`, has `type: stdio`, allows the tool in its `tools:` filter, uses valid template syntax in `arguments`, and avoids referencing sibling parallel steps. | ||
| - **Runtime validation (`conductor run`):** Repeats all static checks and additionally verifies that the tool actually exists on the live connected server. |
There was a problem hiding this comment.
RECOMMENDED: conductor run doesn't repeat all of the static checks. _run_mcp_step() checks server declaration, transport, tool allowlisting, and live tool existence, but it doesn't do parallel-sibling dependency analysis — so a configuration conductor validate would reject can still execute, using a sibling's previously stored output. The current wording implies skipping validation still preserves those guarantees, which isn't true.
Enumerate the checks that actually get repeated at runtime, and state plainly that conductor validate is still necessary for the offline template-reference and parallel-dependency diagnostics.
There was a problem hiding this comment.
Fixed in ec64c8a: the Validation rules section now enumerates what conductor run actually repeats (server declared, stdio transport, tool allowlisted, plus the live-only tool-existence check) and states plainly that template syntax checking and the same-parallel-group reference analysis are validate-only diagnostics — with the caveat that skipping validation forfeits them.
| - `mcp_completed`: contains `agent_name`, `elapsed`, `server`, `tool`, `is_error`, `result_bytes`, `truncated`, and optional `spill_path`. | ||
| - `mcp_failed`: contains `agent_name`, `elapsed`, `server`, `tool`, `error_type`, and a generic redacted `message`. | ||
|
|
||
| Argument values and result payloads are never included in event data, ensuring credentials or sensitive parameters do not leak to event logs or dashboard streams. When a call fails, `mcp_failed` records only the exception class name and a sanitized message; full tracebacks and raw error details are written to debug logs only (`exc_info=True`). The redaction extends downstream: the step re-raises a generic error, so `workflow_failed` and group failure events (`parallel_agent_failed`, `for_each_item_failed`) also carry only the sanitized message — never raw manager or SDK exception text, which can embed argument or result values. |
There was a problem hiding this comment.
RECOMMENDED: Omitting argument/result bodies from events isn't the end-to-end secrets guarantee this section implies it is. For-each key_by values get copied into item_key on MCP lifecycle events, so a key_by: token config exposes that token even if it's also a tool argument. Explicitly returning MCP results through workflow output also publishes them in workflow_completed. Both are intentional, and both contradict the unconditional wording here.
Scope the guarantee to automatic exclusion of argument/result bodies specifically. Add an explicit warning that item identifiers and authored downstream/final outputs stay visible, and advise against sensitive key_by values.
There was a problem hiding this comment.
Fixed in ec64c8a: the guarantee is now scoped to automatic exclusion of argument/result bodies from event payloads. The section explicitly warns that key_by values land on item-scoped mcp_* events as item_key (advising against sensitive identifiers) and that explicitly surfacing a result — via workflow output: or a later step's prompt/arguments — publishes it in workflow_completed and beyond by design. The new diagnostic file's sensitivity is documented alongside.
Add a new workflow step type that calls a single tool on a configured
MCP server directly, with no LLM in the loop:
- Engine-owned lazy stdio connection pool keyed by (server, runtime
working_dir), with per-server asyncio locks, an idle-manager bound,
and unconditional cleanup in finally run()/resume()
- Structured envelope result {content, structured, is_error} in the
workflow context; is_error is routable data, not a step failure
- Redacted event contract: mcp_started/mcp_completed/mcp_failed carry
only metadata (server, tool, argument_keys, elapsed, is_error,
result_bytes, truncated, spill_path) — never argument or result
values — surfaced in console, JSONL, web replay, and fleet summary
- Dispatch in all three engine positions: main loop, parallel groups,
and for_each; fail-fast groups cancel and drain siblings before
pool cleanup
- Static off-network validation: server declared, tool allowlisted,
stdio transport only, argument templates checked
- Dashboard: mcpNode graph node + McpDetail panel (metadata only)
- Docs, examples/mcp-step.yaml, bundled skill references, CHANGELOG
Review-driven fixes for the type: mcp step (PR microsoft#518): - Pool admission is now atomic under the pool guard: an in-flight connect reserves its slot (_mcp_step_pending), so two concurrent first-time connects can no longer both pass a size-only capacity check and overshoot _MCP_STEP_POOL_MAX with nothing evicted. The cap is documented as a soft threshold (busy entries are never evicted mid-call). - Eviction cleanup no longer swallows task cancellation: the evicted manager's close() runs as its own shielded task (close() deliberately absorbs CancelledError while draining), tolerates repeated cancellation, and re-raises CancelledError before any new connect or invoke happens. - The "see debug logs" failure message pointed nowhere: --log-file wires a Rich console, not a Python logging handler. Redacted mcp failures now write the full exception (including cause chains) to a private per-run *.mcp-diagnostics.log next to the run's event log, and the redacted message points at that file. Safe categories keep their authored messages verbatim (timeouts keep their duration; the name-only runtime checks propagate as before). - Structured result keys "outputs"/"errors" are now reserved from the top-level merge: WorkflowContext duck-types group outputs by exactly those keys, so a flattened pair would misclassify a step's output as a group output in every context mode. They stay under output.structured. - call_tool_structured strips server-supplied "truncated"/"spill_path" content-block fields at ingestion; only Conductor's own truncation pass may set them, and a shared typed reader (mcp_truncation_metadata) is the single read-side for live events and synthetic replay alike. - Synthetic replay of parallel/for-each groups no longer publishes saved MCP envelopes in the aggregate outputs: mcp members/items are replayed as the metadata-only event sequence live execution emits. - MCPManager.connect_server gains redact_errors=True for the deterministic step path: connection failures log safe metadata only (no server stderr or traceback at ERROR); provider-facing behavior is unchanged. - Main-loop mcp steps watch interrupt_event across the slot/connect/call waits: a dashboard Stop cancels the in-flight call (never auto-replayed — side effects are unknown) and enters the existing pause flow (agent_paused -> Resume/Kill; CLI menu without a dashboard). Group members mirror LLM members and are reached via group cancellation. - The runtime tool-allowlist check now uses wildcard membership ("*" in tools), exactly matching conductor validate; and validate now parses every nested argument template for Jinja2 syntax errors (reference analysis deliberately swallows TemplateSyntaxError). - Docs: argument coercion describes the actual whole-render YAML parsing (embedded templates can coerce to non-strings); the validation section enumerates which checks runtime repeats vs validate-only ones; the secrets section scopes the no-values guarantee (item_key identifiers and authored downstream outputs stay visible) and documents the diagnostic file; restrictions lists gain settings_dir (rebase follow-up).
Review-driven dashboard fixes for the type: mcp step (PR microsoft#518): - A parallel group's MCP member no longer increments workflow completion on its mcp_completed (the group increments it on parallel_completed) — a one-member MCP group reported 2/1 completed before. - Parallel group members keep their declared step type: workflow_started seeds their nodes from the agents' declared types (both root and sub-workflow initialization), and graph layout picks the renderer from the declared type via a shared flowNodeTypeFor mapping, so a parallel MCP member renders as McpNode and opens McpDetail. - For-each item rows factor MCP metadata into expandability and render it (server, tool, result size with the truncation marker, spill path); items whose tool reported is_error: true stay "completed" but show a visible warning. No raw result body is exposed — none exists on items by design. Static dashboard assets rebuilt (make build-frontend).
d38d71e to
79d09fd
Compare
Jason Robert (jrob5756)
left a comment
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
14 points from the last pass landed: eviction now preserves cancellation, structured results no longer masquerade as group outputs, redacted failures point somewhere real, pool admission counts pending connections, wildcard allowlisting agrees at both boundaries, and the docs now match whole-render parsing and the runtime/offline split, among others.
Three points are still open: one blocking, two recommended.
- Blocking: checkpoint replay still trusts server-supplied truncation/spill markers (
src/conductor/executor/mcp_step.py:121)
Details on all three are inline.
| ( | ||
| b["spill_path"] | ||
| for b in blocks | ||
| if isinstance(b, dict) and isinstance(b.get("spill_path"), str) and b["spill_path"] |
There was a problem hiding this comment.
BLOCKING: Server-supplied truncation and spill markers are still trusted on replay.
manager.py:458-460 now strips spill_path and truncated from newly ingested results, so the live path is closed. But this file still accepts any nonempty spill_path string and truncated=true straight off saved checkpoint content (lines 115-125), and server.py:1085-1104 passes that saved content through unchanged and publishes the markers. Since the original implementation persisted these fields into checkpoints, an existing checkpoint can still replay a server-supplied string as trusted metadata without any tampering.
Needs: suppress untrusted legacy markers on replay, or distinguish persisted Conductor-generated metadata from server extensions before publishing it.
There was a problem hiding this comment.
Fixed in b300d35. _synth_mcp_pair no longer republishes markers from a stored envelope: synthetic mcp_completed events now always report truncated: false / spill_path: null, and mcp_truncation_metadata is documented as the live-path-only reader — its trust comes from the ingestion stripping, which a persisted checkpoint may predate. The stored envelope itself stays intact in the workflow context for routing and templates; only the published synthetic event loses the badge. The test that pinned the old behavior was updated, and a new test asserts suppression on the standalone, parallel, and for-each synthetic paths (all three converge on _synth_mcp_pair).
| if self._interrupt_event is not None: | ||
| self._interrupt_event.clear() | ||
| continue | ||
| if self._web_dashboard is not None: |
There was a problem hiding this comment.
RECOMMENDED: Stop handling still misses groups and can auto-replay a call.
Standalone invocations now race against interrupt_event (lines 1950-1968, 2129-2165), so Stop reaches those. But the parallel and for-each callers at 7039-7043 and 7574-7578 still don't wire interruption into their group waits, so Stop is silent there. And this line re-enters an interrupted standalone step automatically when no browser is connected — the shared pause handler does the same on disconnect — so an interrupted call can replay without anyone actually deciding to resume it. Applies to both run and resume.
Needs: connect group execution to the Stop/pause flow, and require an explicit resume decision before repeating an interrupted MCP call, including the no-client and disconnect cases.
There was a problem hiding this comment.
The auto-replay half is fixed in 78c55ec. WebPauseOutcome now carries an explicit reason (resume / guidance / disconnect / unavailable), and an interrupted mcp call is re-entered only on an explicit resume decision: a dashboard Resume/guidance, or the CLI interrupt menu (every menu outcome is a decision). On a mid-pause disconnect or a dashboard with zero connected clients the run parks instead of replaying: a new _McpStepOutcomeUncertain (an InterruptError subclass, so workflow_failed is flagged stopped_by_user and a failure checkpoint is saved) makes conductor resume the explicit at-least-once boundary. No mcp_failed is emitted — the tool reported no failure, the call's outcome is simply unknown — and no agent_resumed on the parking path; an explicit Resume completed in the same wait batch wins over a simultaneous disconnect. The LLM auto-resume behavior and its event stream are unchanged, since re-running an LLM agent only costs tokens.
The group half I deliberately did not change here. No step type — LLM agents included — receives a mid-call interrupt inside parallel/for-each today: Stop takes effect at the next between-step check, and Kill works via engine-task cancellation. Wiring Stop into group waits is a cross-cutting change (cancel one member or the whole group, draining siblings, fail_fast interaction, N concurrent pause UIs) that deserves its own issue rather than an mcp-only divergence. The cancellation paragraph in docs/workflow-syntax.md now states the exact semantics, including the no-implicit-replay rule.
| ensureNode(ctx.nodes, pg.name, 'parallel_group'); | ||
| ctx.groupProgress[pg.name] = { total: pg.agents.length, completed: 0, failed: 0 }; | ||
| for (const agentName of pg.agents) ensureNode(ctx.nodes, agentName, 'agent'); | ||
| for (const agentName of pg.agents) ensureNode(ctx.nodes, agentName, agentTypes.get(agentName) || 'agent'); |
There was a problem hiding this comment.
RECOMMENDED: Static nested MCP members still carry the generic detail type.
Root-level graph mapping and fresh initialization now use the declared type. But buildStaticChildContext (line 631) still seeds parallel members as 'agent', normal subworkflow startup reuses those placeholders (1430-1443), and ensureNode (532-539) doesn't update an existing node's type. So nested members still end up typed 'agent' here, and DetailPanel.tsx:59-71 routes them to AgentDetail instead of McpDetail.
Needs: preserve declared types when building static child contexts, and update reused placeholders during initialization.
There was a problem hiding this comment.
Fixed in 5a245b7. buildStaticChildContext now seeds parallel members with their declared types (the same declared-type map the root and child workflow_started handlers already use), and the child workflow_started re-syncs every declared agent's node type from the runtime topology onto reused placeholder nodes — ensureNode intentionally still never updates a type, so untyped event paths can't downgrade a typed node, and the sync walks only ctx.agents, leaving group nodes untouched. Vitest coverage added for both halves: declared types in static previews, and a deliberately stale placeholder being corrected when the real workflow_started lands on it.
…eplay The synthetic replay path built mcp_completed events from checkpoint- restored envelopes and forwarded their truncated/spill_path content-block fields as trusted Conductor metadata. Those markers are only trustworthy on the live path, where call_tool_structured strips server-supplied fields of those names at ingestion: a checkpoint written before the stripping existed can carry a server-supplied spill_path, and replaying it presents server-controlled data as Conductor-generated metadata. Synthetic events now report no truncation (the stored envelope itself stays intact in the workflow context for routing and templates), and mcp_truncation_metadata is documented as the live-path-only reader.
A Stop cancelling an in-flight mcp tool call used to be followed by an automatic re-execution whenever no human resolved the pause: every browser client disconnecting mid-pause returned the same pause outcome as an explicit Resume click, and a dashboard with zero connected clients auto-resumed. For a side-effecting tool call that silently repeats work whose external outcome is unknown. WebPauseOutcome now carries an explicit reason (resume/guidance/disconnect/unavailable), and the disconnect arm no longer emits agent_resumed — the LLM caller emits it there when it auto-resumes, preserving its event stream. An explicit Resume completed in the same wait batch wins over a simultaneous disconnect. The mcp dispatch re-enters the step only on resume/guidance (or the CLI interrupt menu, which is an explicit decision by definition); on disconnect or a clientless dashboard it raises _McpStepOutcomeUncertain, an InterruptError subclass, so the run stops flagged stopped_by_user with a failure checkpoint and conductor resume becomes the explicit at-least-once re-execution boundary. No mcp_failed is emitted: the tool reported no failure, the call's outcome is simply unknown.
Members of parallel groups inside statically previewed sub-workflows were seeded as generic 'agent' nodes, and ensureNode never updates an existing node's type, so the runtime workflow_started landing on the reused placeholder never corrected them either — nested mcp members ended up routed to AgentDetail instead of McpDetail. buildStaticChildContext now seeds parallel members with their declared types (the same map the root and child workflow_started handlers already use), and the child workflow_started re-syncs every declared agent's node type from the runtime topology onto reused placeholder nodes, leaving group nodes untouched.
Resolves the CHANGELOG.md conflict from the 0.1.37 release cut: the `type: mcp` entry moves from the promoted 0.1.37 section back under `## [Unreleased]`, and main's released 0.1.37 notes are kept intact. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jason Robert (jrob5756)
left a comment
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
LGTM, thanks for contributing!
3 participants
Closes #392.
What
A new workflow step type,
type: mcp, that calls a single tool on a configured MCP server directly — no LLM in the loop, zero prompt tokens, fully deterministic. The tool result lands in the workflow context as a structured envelope and can drive routing, including branching on the tool'sisErrorflag.Key design decisions
MCPManagerpool keyed by (server, runtime working dir), with per-serverasyncio.Lockserialization (so parallel/for_each members calling the same server don't interleave), a bounded pool with idle eviction, and unconditional cleanup in afinallyonrun()/resume(). Fail-fast groups cancel and drain in-flight siblings before pool cleanup.is_erroris data, not failure. A tool error is routable data in the envelope (likeexit_codefor script steps). Only transport-level failures fail the step.mcp_started/mcp_completed/mcp_failedcarry only metadata (server, tool, argument keys, elapsed, is_error, result_bytes, truncated, spill_path) — never argument or result values — in console, JSONL, web replay, and fleet summary. The existing string-basedMCPManager.call_toolis untouched; a newcall_tool_structuredis added alongside it.conductor validatechecks that the server is declared, the tool is allowlisted by the server'stools:filter, the transport is stdio (a clear error for http/sse — deferred to a future phase), and argument templates parse.{content: [...], structured: {...}|null, is_error: bool}—structuredkeys are merged on top of the envelope without allowing the reserved envelope keys to be shadowed.Coverage
mcpNodegraph node +McpDetailpanel (metadata only)workflow-syntax.md,mcp-tools.md),examples/mcp-step.yaml, bundled skill references, CHANGELOGworking_dirVerification
make checkclean (ruff + ty; 6 pre-existing unrelated warnings)make test-frontend: 159 passed;make build-frontend;make validate-examplesis_error,--web-bgdashboard, resume after a transport failure, and fleet visibility