fix(copilot): address review findings on runtime restart state machine

Blocking fixes (PR #484 review):
- _restart_spawned_runtime no longer publishes the rebuilt client until
  it has actually started: _client/_started are invalidated first, so a
  failed start() (e.g. OOM at spawn) leaves the provider correctly
  believing no client is started, instead of silently disabling
  dead-runtime recovery for the rest of the process.
- The consecutive-restart cap is now checked before incrementing the
  counter and is never left stale: the cap can no longer be tripped
  after zero actual restarts, the giving-up message reports the real
  restart count, and close() resets the counter so a cached provider
  isn't permanently wedged after a workflow crash-loops once.
- Replaced the unfalsifiable cap-message assertion in
  test_copilot_runtime_recovery.py with one that pins the rendered
  clause and asserts the cap actually prevents the next rebuild.
- Added tests/test_providers/conftest.py: an autouse fixture clearing
  COPILOT_PROVIDER_RUNTIME_URL/TOKEN so the runtime-recovery tests pass
  regardless of the developer's/CI runner's environment.
- Added a regression test covering the corrupted-state bug: when the
  rebuilt client's start() raises, _started must end up False and a
  later _ensure_client_started() must re-attempt start().

Recommendations applied:
- _runtime_unavailable_error now distinguishes a confirmed-dead process
  (poll() returned an exit code) from a broken connection to a still-
  alive process, instead of always claiming the process "died" and
  suggesting NODE_OPTIONS.
- Client teardown during restart, and session.disconnect() in the
  per-agent finally block, now log a warning on failure instead of
  silently swallowing the exception (a leaked child / stranded session
  is diagnostically useful, especially given this PR's own OOM focus).
- The session.error ProviderError path is now also routed through dead-
  runtime classification when retryable, instead of always surfacing a
  generic "Copilot SDK error" message that hides an exit-code 137 OOM
  kill.
- Narrowed _spawned_runtime_process's return type from Any | None to
  subprocess.Popen[bytes] | None, matching the isinstance check the
  body already performs and the SDK's own annotation.
- Added a one-time warning when a spawned, started client has no usable
  _cli_process handle, so a future SDK rename surfaces instead of
  silently degrading recovery to a no-op.
- Fixed the inverted _FakeClient docstring/comments describing mock
  auto-vivification as looking "live" when it in fact reads as dead.
- Scoped the restart-counter-reset comment to agent execution (several
  auxiliary paths increment without resetting).
- Updated CHANGELOG.md, docs/configuration.md and AGENTS.md to name the
  restart cap (2, fixed, non-configurable), correct the "endlessly
  retrying" overstatement, and scope the SDK-boundary claim to
  agent-execution; documented the _cli_process vs _process split.

Recommendations skipped (not applied): #5 (_interrupted_session reset +
disclosure wording), #6 (max_session pre-flight), #12 (Liveness enum),
#13 (_RestartBudget value type), #17 (per-generation client tracking
for parallel groups), #18 (additional missing tests beyond the one
added for finding #1), #19 (collapsing except clauses), #20 (extracting
shared helpers) -- all correctness-neutral hardening/refactors judged
to grow the diff beyond what this pass should touch; pyproject.toml
dependency cap was also left alone as an unrelated, broader change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: Jason Robert

AGENTS.md

@@ -151,7 +151,7 @@ step-by-step checklist.
   - `base.py` - `AgentProvider` ABC defining `execute()`, `validate_connection()`, `close()`
   - `_output_shape.py` - `normalize_agent_output(content, schema)` — the single entry point providers call before `validate_output` (issue #343). It raises `ValidationError` when the parsed response is not a JSON object (a bare `42`/`null`/array), because `validate_output` would otherwise either raise `TypeError` from a membership test (numbers, booleans, null) or report a misleading "missing required field" (strings, arrays). It then applies `unwrap_scalar_wrappers`: fires only when the schema declares `string`/`number`/`boolean`, a `dict` arrived, and **exactly one** candidate slot has the expected type. Candidate slots are the field's own name plus the generic `value`/`result` keys, deduped so a field literally named `value` or `result` isn't rejected as ambiguous against itself. Two matches count as ambiguous; any other key shape is ignored. Both are left untouched (same object identity) so the caller re-prompts rather than guessing — this is what stops `{"answer": {"error": "..."}}` being laundered into an answer. Every unwrap logs a warning, naming discarded sibling keys when there are any. Kept out of `executor/output.py` on purpose — see the note there.
   - `_recovery_prompt.py` - `build_parse_recovery_prompt(...)` — the plain-text re-prompt shared by Copilot and Hermes (issue #343). Both providers correct an unusable response the same way (error + truncated response + rendered schema, with distinct schema-failure vs syntax-failure wording), and that text is covered by the provider-parity rule, so it lives in one place instead of two copies free to drift. Claude is deliberately not a caller: it re-prompts through its `emit_output` tool and never echoes the schema, so its instruction text stays in `claude.py::_build_recovery_instruction`.
-  - `copilot.py` - GitHub Copilot SDK implementation. By default spawns a nested `copilot` runtime via `CopilotClient()` (in `_build_client`, called from `_ensure_client_started`). When a runtime connection is resolved (`runtime.provider.runtime_url` or `COPILOT_PROVIDER_RUNTIME_URL`, optional `runtime_token` / `COPILOT_PROVIDER_RUNTIME_TOKEN`), it instead builds `CopilotClient(connection=RuntimeConnection.for_uri(url, connection_token=token))` to connect to an already-running `copilot --headless` process; the SDK skips spawning for URI connections and its `stop()` leaves the externally-owned server running. `_resolve_runtime_connection()` reads YAML first, then the namespaced env var (env activates on its own — the zero-YAML path for external orchestrators). Runtime transport can be combined with custom model-provider routing.
+  - `copilot.py` - GitHub Copilot SDK implementation. By default spawns a nested `copilot` runtime via `CopilotClient()` (in `_build_client`, called from `_ensure_client_started`). When a runtime connection is resolved (`runtime.provider.runtime_url` or `COPILOT_PROVIDER_RUNTIME_URL`, optional `runtime_token` / `COPILOT_PROVIDER_RUNTIME_TOKEN`), it instead builds `CopilotClient(connection=RuntimeConnection.for_uri(url, connection_token=token))` to connect to an already-running `copilot --headless` process; the SDK skips spawning for URI connections and its `stop()` leaves the externally-owned server running. `_resolve_runtime_connection()` reads YAML first, then the namespaced env var (env activates on its own — the zero-YAML path for external orchestrators). Runtime transport can be combined with custom model-provider routing. `_ensure_client_started()` also detects a spawned runtime whose child process has died (issue #483, `_runtime_is_dead()` polling the SDK's private `_cli_process` handle) and rebuilds the client under `_start_lock` (`_restart_spawned_runtime`) before returning, so the next SDK call lands on a fresh runtime; the rebuild invalidates `_client`/`_started` before constructing and starting the replacement, so a failed rebuild (e.g. OOM at spawn) cannot leave the provider believing a never-started client is started. A fixed, non-configurable cap (`_MAX_CONSECUTIVE_RUNTIME_RESTARTS`) on consecutive restarts with no intervening successful call prevents an infinite crash loop; a broken connection to an externally-owned runtime is never respawned. `_spawned_runtime_process` reads `_cli_process` (the spawned-child handle, `None` for URI and FFI connections) while `_fix_pipe_blocking_mode` reads `_process` (the transport handle — a `SocketWrapper` in TCP mode, an `_FfiProcessAdapter` with its own `poll()` in FFI mode); both are correct for their own purpose, and unifying the two reads onto `_process` would make FFI mode look like a killable child process.
   - `claude.py` - Anthropic Claude API provider using `pydantic-ai` (`AnthropicModel`) and the internal `_pydantic_ai` package (`converters`, `events`, `mcp_toolset`, `agent_builder`, `interrupt`, `retry`, `structured_output`, `usage`)
   - `claude_agent_sdk.py` - Claude Agent SDK implementation (uses `claude-agent-sdk` package)
   - `factory.py` - Provider instantiation

CHANGELOG.md

@@ -65,23 +65,27 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   paths now use the same bounded retry that `write_run_record` already used
   for its own `os.replace`.
 - **The Copilot provider now recovers automatically when its nested runtime
-  process dies** (#483), instead of endlessly retrying against a dead
-  process and reporting a misleading "Check that copilot CLI is installed
-  and authenticated" error. A dead spawned runtime is now detected via
+  process dies** (#483), instead of retrying against a dead process with
+  a misleading "Check that copilot CLI is installed and authenticated"
+  error. A dead spawned runtime is now detected via
   `subprocess.Popen.poll()` on the SDK's own child handle and via explicit
-  recognition of `BrokenPipeError` / `ConnectionResetError` at the SDK
-  boundary (including during idle-recovery "continue" prompts, which
-  previously burned every recovery attempt and were reported as a stuck
-  *agent* rather than a dead *process*). Recovery rebuilds the SDK client
-  the next time it is needed, so the existing retry loop lands its next
-  attempt on a fresh runtime with no change to retry-loop shape; a runtime
-  that keeps dying without a single successful call in between fails fast
-  after a small number of consecutive restarts rather than looping forever,
-  while a long-running, otherwise-healthy workflow can restart it as many
-  times as needed. A broken connection to an **externally-owned** runtime
-  (`runtime_url` / `COPILOT_PROVIDER_RUNTIME_URL`) is treated differently:
-  it is never retried or respawned, since the orchestrator that owns that
-  runtime is responsible for its health checks and restarts.
+  recognition of `BrokenPipeError` / `ConnectionResetError` at the
+  agent-execution SDK boundary (including during idle-recovery "continue"
+  prompts, which previously burned every recovery attempt and were
+  reported as a stuck *agent* rather than a dead *process*). Recovery
+  rebuilds the SDK client the next time it is needed, so the existing
+  retry loop lands its next attempt on a fresh runtime with no change to
+  retry-loop shape; a runtime that keeps dying without a single
+  successful call in between fails fast after 2 consecutive restarts
+  (a fixed, non-configurable cap — with the default `max_attempts` of 3,
+  a single agent execution can only trigger 2 restarts on its own, so the
+  cap mainly bites across agents in the same workflow) rather than
+  looping forever, while a long-running, otherwise-healthy workflow can
+  restart it as many times as needed. A broken connection to an
+  **externally-owned** runtime (`runtime_url` /
+  `COPILOT_PROVIDER_RUNTIME_URL`) is treated differently: it is never
+  retried or respawned, since the orchestrator that owns that runtime is
+  responsible for its health checks and restarts.
 
 ## [0.1.33](https://github.com/microsoft/conductor/compare/v0.1.32...v0.1.33) - 2026-08-18
 

docs/configuration.md

@@ -339,13 +339,18 @@ conductor run review.yaml                 # connects; spawns no nested runtime
 - Closing the provider does **not** terminate the external runtime — the
   SDK only shuts down runtimes it spawned itself, so the orchestrator-owned
   server keeps running. The orchestrator is also responsible for runtime
-  health checks and restarts: a broken connection to an external runtime
-  fails the affected agent immediately (`is_retryable=false`) and is never
+  health checks and restarts: a **lost connection** to an external runtime
+  (a `BrokenPipeError` or `ConnectionResetError` at the SDK boundary) fails
+  the affected agent immediately (`is_retryable=false`) and is never
   retried or respawned by Conductor. This differs from the default spawned
   runtime, which Conductor restarts automatically after a detected crash
   (a dead child process, or a `BrokenPipeError`/`ConnectionResetError` at
-  the SDK boundary) and retries against, up to a small consecutive-failure
-  cap.
+  the SDK boundary) and retries against, up to a fixed cap of 2 consecutive
+  restarts without an intervening successful call (not configurable via
+  YAML or an environment variable). Note this covers a *lost* connection
+  only; a *failed initial connect* to an external runtime (e.g. it is not
+  reachable at all) is not classified by this mechanism and surfaces as a
+  generic SDK error instead.
 - Runtime-spawn-only options (custom CLI path, injected env, etc.) do not
   apply when connecting to an existing runtime.
 

src/conductor/providers/copilot.py

@@ -146,10 +146,14 @@ class RetryConfig:
 
 # Cap on consecutive spawned-runtime restarts with no intervening successful
 # SDK call (issue #483). This is a death-loop guard, not a per-agent budget:
-# any successful call resets the counter to 0 (see ``_execute_with_retry``),
-# so a long-running workflow that legitimately restarts many times over hours
-# is unaffected — only a runtime that dies again before ever succeeding trips
-# the cap.
+# any successful agent-execution call resets the counter to 0 (see
+# ``_execute_with_retry``), so a long-running workflow that legitimately
+# restarts many times over hours is unaffected via that path — only a runtime
+# that dies again before ever succeeding trips the cap. Auxiliary paths that
+# also call ``_ensure_client_started`` (``validate_connection``,
+# ``execute_dialog_turn``, ``get_max_prompt_tokens``, ``get_model_pricing``,
+# ``list_models``, ``_validate_reasoning_effort_for_model``) can increment the
+# counter without resetting it on success.
 _MAX_CONSECUTIVE_RUNTIME_RESTARTS = 2
 
 
@@ -419,6 +423,9 @@ def __init__(
         # SDK call. Reset to 0 on every successful call in
         # ``_execute_with_retry``; see ``_MAX_CONSECUTIVE_RUNTIME_RESTARTS``.
         self._consecutive_runtime_restarts = 0
+        # Guards a one-time warning when a spawned, started client has no
+        # ``_cli_process`` handle (see ``_spawned_runtime_process``).
+        self._warned_missing_runtime_handle = False
         self._idle_recovery_config = idle_recovery_config or IdleRecoveryConfig()
         self._temperature = temperature
         self._default_max_agent_iterations = max_agent_iterations
@@ -1523,9 +1530,21 @@ async def _execute_sdk_call(
             finally:
                 # Disconnect session unless it was kept alive for follow-up
                 if not session_destroyed:
-                    await session.disconnect()
+                    try:
+                        await session.disconnect()
+                    except asyncio.CancelledError:
+                        raise
+                    except Exception:
+                        logger.warning(
+                            "Failed to disconnect Copilot session for agent '%s' during "
+                            "cleanup; continuing.",
+                            agent.name,
+                            exc_info=True,
+                        )
 
-        except ProviderError:
+        except ProviderError as e:
+            if e.is_retryable and self._runtime_is_dead():
+                raise self._runtime_unavailable_error(e) from e
             raise
         except ValidationError:
             # Deterministic failures: a configuration error (e.g. unsupported
@@ -2468,7 +2487,7 @@ async def _ensure_client_started(self) -> None:
             elif self._runtime_is_dead():
                 await self._restart_spawned_runtime()
 
-    def _spawned_runtime_process(self) -> Any | None:
+    def _spawned_runtime_process(self) -> subprocess.Popen[bytes] | None:
         """Return the subprocess this provider owns and spawned, else None.
 
         Returns ``None`` (rather than guessing) for every mode where this
@@ -2488,8 +2507,29 @@ def _spawned_runtime_process(self) -> Any | None:
             return None
         if self._client is None:
             return None
+        # ``_cli_process`` is the spawned-child handle (a real OS process,
+        # None for URI and FFI connections). This is deliberately distinct
+        # from ``_fix_pipe_blocking_mode``'s ``_process``, which is the
+        # transport handle instead (a ``SocketWrapper`` in TCP mode, an
+        # ``_FfiProcessAdapter`` with its own ``poll()`` in FFI mode) --
+        # unifying the two reads onto ``_process`` would make FFI mode look
+        # like a killable child process.
         process = getattr(self._client, "_cli_process", None)
         if not isinstance(process, subprocess.Popen):
+            # We spawned something (not external, client built and started),
+            # so a handle should exist. The one benign explanation is FFI
+            # in-process mode, which has no OS child process at all; anything
+            # else here is a capability regression (e.g. an SDK rename of
+            # ``_cli_process``) that would otherwise silently disable
+            # dead-runtime recovery with no diagnostic.
+            if self._started and not self._warned_missing_runtime_handle:
+                self._warned_missing_runtime_handle = True
+                logger.warning(
+                    "Spawned Copilot runtime has no usable _cli_process handle. "
+                    "This is expected in FFI in-process mode; otherwise it may "
+                    "indicate an incompatible Copilot SDK version, which would "
+                    "silently disable dead-runtime restart recovery (issue #483)."
+                )
             return None
         return process
 
@@ -2529,32 +2569,42 @@ def _runtime_unavailable_error(self, exc: BaseException) -> ProviderError:
 
         process = self._spawned_runtime_process()
         exit_code = process.poll() if process is not None else None
-        exit_code_desc = f" (exit code {exit_code})" if exit_code is not None else ""
+        if exit_code is not None:
+            return ProviderError(
+                f"The Copilot runtime process died (exit code {exit_code}): {exc}",
+                suggestion=(
+                    "The nested Copilot runtime will be restarted automatically on the "
+                    "next attempt. If this recurs, it may indicate the runtime process "
+                    "is running out of memory; try setting "
+                    'NODE_OPTIONS="--max-old-space-size=8192" in the environment running '
+                    "conductor."
+                ),
+                is_retryable=True,
+            )
+
         return ProviderError(
-            f"The Copilot runtime process died{exit_code_desc}: {exc}",
+            f"The connection to the Copilot runtime broke: {exc}",
             suggestion=(
-                "The nested Copilot runtime will be restarted automatically on the "
-                "next attempt. If this recurs, it may indicate the runtime process "
-                "is running out of memory; try setting "
-                'NODE_OPTIONS="--max-old-space-size=8192" in the environment running '
-                "conductor."
+                "The runtime process is still running, so this was a transport "
+                "failure rather than a crash. The connection will be re-established "
+                "automatically on the next attempt."
             ),
             is_retryable=True,
         )
 
     async def _restart_spawned_runtime(self) -> None:
         """Rebuild the Copilot client after detecting a dead spawned runtime.
 
-        Must be called while holding ``self._start_lock``. Increments the
-        consecutive-restart counter (reset on any successful SDK call, see
-        ``_execute_with_retry``) and fails fast if a runtime keeps dying
-        before ever succeeding, rather than looping forever.
+        Must be called while holding ``self._start_lock``. Checks the
+        consecutive-restart cap (reset on any successful SDK call, see
+        ``_execute_with_retry``) before incrementing it and fails fast if a
+        runtime keeps dying before ever succeeding, rather than looping
+        forever.
         """
-        self._consecutive_runtime_restarts += 1
-        if self._consecutive_runtime_restarts > _MAX_CONSECUTIVE_RUNTIME_RESTARTS:
+        if self._consecutive_runtime_restarts >= _MAX_CONSECUTIVE_RUNTIME_RESTARTS:
             raise ProviderError(
                 "The Copilot runtime process died and was restarted "
-                f"{self._consecutive_runtime_restarts - 1} times in a row without a "
+                f"{_MAX_CONSECUTIVE_RUNTIME_RESTARTS} times in a row without a "
                 "single successful call. Giving up rather than restarting again.",
                 suggestion=(
                     "Check the runtime for a crash loop (e.g. persistent OOM). Try "
@@ -2564,6 +2614,8 @@ async def _restart_spawned_runtime(self) -> None:
                 is_retryable=False,
             )
 
+        self._consecutive_runtime_restarts += 1
+
         old_process = self._spawned_runtime_process()
         old_exit_code = old_process.poll() if old_process is not None else None
         logger.warning(
@@ -2576,12 +2628,30 @@ async def _restart_spawned_runtime(self) -> None:
 
         # Best-effort teardown of the dead client. stop() attempts a graceful
         # RPC shutdown plus process-exit waits against a corpse, so it is
-        # bounded rather than allowed to hang recovery.
-        with contextlib.suppress(Exception, asyncio.TimeoutError):
+        # bounded rather than allowed to hang recovery. Failures are logged
+        # rather than silently swallowed: a StopError here means a runtime
+        # child survived both terminate and kill, i.e. a leaked process.
+        try:
             await asyncio.wait_for(self._client.stop(), timeout=10.0)
+        except asyncio.CancelledError:
+            raise
+        except Exception:
+            logger.warning(
+                "Teardown of the dead Copilot client failed; continuing with the "
+                "restart. A runtime child process may have been leaked.",
+                exc_info=True,
+            )
+
+        # Invalidate the old client before rebuilding: if the new client's
+        # start() raises (e.g. OOM at spawn), we must not be left believing
+        # a never-started client is started, which would silently skip
+        # recovery on the next call.
+        self._client = None
+        self._started = False
 
-        self._client = self._build_client()
-        await self._client.start()
+        new_client = self._build_client()
+        await new_client.start()
+        self._client = new_client
         self._started = True
         self._fix_pipe_blocking_mode()
 
@@ -2937,6 +3007,7 @@ async def close(self) -> None:
                 await self._client.stop()
         self._client = None
         self._started = False
+        self._consecutive_runtime_restarts = 0
         self._call_history.clear()
         self._retry_history.clear()