Skip to content

fix: add per-agent verbose attribution for parallel and for_each execution - #20

Closed
e-s (e-s-gh) wants to merge 2 commits into
microsoft:mainfrom
e-s-gh:fix/issue-16-agent-attribution
Closed

fix: add per-agent verbose attribution for parallel and for_each execution#20
e-s (e-s-gh) wants to merge 2 commits into
microsoft:mainfrom
e-s-gh:fix/issue-16-agent-attribution

Conversation

@e-s-gh

@e-s-gh e-s (e-s-gh) commented Mar 5, 2026

Copy link
Copy Markdown

Summary

  • add optional agent_name plumbing from _execute_sdk_call() into _send_and_wait() and _log_event_verbose()
  • prefix verbose provider events with [agent_name] for tool start/complete, reasoning, sub-agent start/complete, and turn start
  • qualify for_each execution agent names per item via model_copy(update={"name": f"{base}[{key}]"})
  • execute for_each items with the qualified agent and emit streaming event agent_name using the qualified value
  • add a runnable example workflow at examples/for-each-agent-attribution.yaml

Why

Verbose logs from concurrent agent runs were interleaved without attribution, making debugging and grep-based analysis impractical for parallel and for_each execution.

Testing

  • uv run pytest tests/test_providers/test_copilot.py tests/test_engine/test_event_emission.py -q
  • uv run pytest tests/test_providers -q
  • uv run conductor validate examples/for-each-agent-attribution.yaml

Added Coverage

  • provider verbose logging now tested for [agent] prefix output
  • _send_and_wait test verifies agent_name is forwarded to _log_event_verbose
  • for_each tests verify per-item qualified names in provider call history for both index keys and key_by values

Fixes #16

Jason Robert (jrob5756) pushed a commit that referenced this pull request Aug 24, 2026
Jason Robert
fix(copilot): address review findings on runtime restart state machine
2600d43 
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>
Jason Robert (jrob5756) pushed a commit that referenced this pull request Aug 24, 2026
Jason Robert
fix(copilot): address review findings on runtime restart state machine
1aea23e 
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>
Jason Robert (jrob5756) added a commit that referenced this pull request Aug 24, 2026
@jrob5756 Jason Robert
fix(copilot): recover from a dead spawned Copilot runtime process (#484)
134b735 
* fix(copilot): recover from a dead spawned Copilot runtime process

Detect when the nested Copilot runtime subprocess has died (broken
pipe/connection reset, or a check before sending an idle-recovery
prompt) and transparently restart it on the next attempt instead of
surfacing a confusing stuck-agent error. Externally-owned runtimes
(runtime_url) are never restarted here -- that failure is reported as
non-retryable so the owning orchestrator can act. A consecutive
restart counter (reset on any successful SDK call) caps restart
attempts so a runtime that keeps dying before ever succeeding fails
fast rather than looping forever.

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

* 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>

---------

Co-authored-by: Jason Robert <jasonrobert@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add agent attribution to verbose logs for parallel and for-each execution

2 participants

@e-s-gh @jrob5756