fix(coordination): close RealRuntime._current_instance parallel-invoke validation race (#40)

Thread the per-branch agent instance into RealRuntime._translate and delete the shared self._current_instance, fixing a parallel-invoke validation cross-talk race (DP-004 violation). Deterministic regression test + ADR-010. Live-verified via anthropic-oauth: pre-fix 4/4 runs misattribute 'AgentC cannot invoke' onto AgentA/AgentB branches; post-fix clean. Closes #40.

Author: rezaho

CHANGELOG.md

@@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [Unreleased]
 
 ### Fixed
+- **`RealRuntime`: parallel-invoke validation cross-talk race fixed; per-branch agent instance is threaded, not shared** (`coordination/execution/real_runtime.py`; issue #40). `RealRuntime` is constructed once per `Orchestra.run()` and the orchestrator dispatches `step()` for every runnable branch as a concurrent `asyncio.Task` on that one shared object. `step()` stashed the per-branch agent instance on `self._current_instance` *before* the `execute_step` await and `_translate()` read it back *after* the await to feed `ValidationProcessor.validate_coordination_action(agent=...)`. Under `parallel_invoke` fan-out a sibling branch overwrote the shared attribute during the await, so a branch's coordination action was validated against a **different** agent's identity — and thus its outgoing topology edges — producing a fabricated, misattributed `"Agent <X> cannot invoke: [...]"` failure (latent when fanned-out workers shared identical outgoing edges; surfaced non-deterministically when they differed). A direct DP-004 (branch isolation) violation. Fix: thread the instance as an explicit parameter into `_translate` (`agent=instance`) and delete `self._current_instance`, so the shared runtime holds zero per-step mutable state — conforming the lone outlier to the file's existing convention (`execute_step`/`_build_content_only_diagnostic` already take `instance` explicitly). Public surface unchanged: the `Runtime` Protocol and `step(branch)` signature are untouched; only the private `_translate` signature changes. Regression guard: a deterministic no-LLM test (`tests/coordination/orchestrator/test_real_runtime_parallel_race.py`) forces the racy interleave via `asyncio.gather` + a yielding `execute_step` — RED on the pre-fix code, GREEN after. See ADR-010.
 - **Anthropic adapters: empty-but-SUCCESSFUL stream terminals become typed classified errors (or the truncation placeholder) instead of an UNKNOWN ValidationError** (`models/adapters/anthropic_oauth.py`, `models/adapters/anthropic.py`, `models/adapters/streaming.py`, `agents/exceptions.py`). Sequel to the stream-failure entry below, which covered failed streams but left empty output synonymous with truncation: an HTTP-200 stream that legitimately produced no content — stop_reason `refusal`, an empty `end_turn`, an empty `model_context_window_exceeded`, or a stream that closed without any terminal — still constructed `HarmonizedResponse(content=None)`, died in the model validator, and was wrapped by the generic handler as a non-retryable `MODEL_API_UNKNOWN_ERROR` with the provider's terminal signal destroyed (this crashed a production Spren daemon on every boot-replay of one parked event). Four changes: (1) all three SSE readers (the OAuth adapter's sync + async accumulators and the shared `AnthropicStreamAccumulator`) capture `message_delta`'s nullable `stop_details` — decoration for error messages, never branched on (the documented contract: branch on `stop_reason`, not `stop_details`); (2) the finish_reason normalization widens to BOTH deterministic-truncation terminals — `max_tokens` AND `model_context_window_exceeded` (live by default on Sonnet 4.5+) normalize to `length`, so an empty context-window-exceeded response takes the existing truncation placeholder instead of being misclassified transient; (3) both adapters' `harmonize_response` generalize the empty branch — zero text, zero tool calls, zero thinking either takes the `length` placeholder or raises `ModelAPIError.from_provider_response` with a new `empty_completion_payload` marker (shared helper in `streaming.py`; deliberately NOT the in-stream `error` key shape, which keeps firing first and separately) carrying only `stop_reason` + `stop_details`; (4) a new status-less arm in `from_provider_response` classifies by stop_reason — `refusal` → new **`REFUSAL`** classification, non-retryable, message carries the refusal fact plus `stop_details` category/explanation when present; empty `end_turn` → new **`EMPTY_COMPLETION`** classification, non-retryable (Anthropic guidance: don't retry empty responses without modification; the suggested action says to send a modified request / continuation prompt); `stop_sequence` / unknown terminals / no terminal at all → `EMPTY_COMPLETION` retryable. Enum choice: `REFUSAL` is its own value rather than a flavor of `EMPTY_COMPLETION` because a content-policy decline and a degenerate empty stream demand different downstream handling, and consumers must not parse message text to tell them apart; both values are additive (`is_critical` membership unchanged; both fall to `get_error_action`'s TERMINAL default — the pre-existing PARTIAL_FAILURE pattern — while the adapter retry ladder reads `is_retryable`). Recorded, not fixed: on the API-key adapter's NON-streaming path `base.py`'s generic handlers re-wrap the typed raise (message survives, classification degrades to UNKNOWN in the returned `ErrorResponse`) — a strict improvement over the ValidationError it replaces; the streaming path (`arun_streaming`) propagates it typed. The OAuth adapter's thinking-only latent gap (thinking set, no text/tool calls, non-length terminal → still a ValidationError) stays recorded-open and unreachable there; the API-key twin already closed it.
 - **Anthropic adapters: stream failures and empty completions no longer crash harmonization** (`models/adapters/anthropic_oauth.py`, `models/adapters/anthropic.py`, `agents/exceptions.py`). Production failure (Spren daemon, 2026-06-11): an in-stream SSE `error` event (Anthropic delivers stream failures under HTTP 200 — `overloaded_error` ≙ HTTP 529) was silently dropped by the OAuth adapter's SSE accumulator, the empty result harmonized, and `HarmonizedResponse`'s own validator rejected it — surfacing as an opaque `MODEL_API_UNKNOWN_ERROR` ValidationError with the provider's real message destroyed. Three changes restore the contract "every stream outcome maps to either a valid `HarmonizedResponse` or a typed classified `ModelAPIError`": (1) both stream readers (sync + async) capture `type:"error"` events and stop reading (partials are discarded by design — recovery is a new request, and partial `tool_use` must never execute; the discarded-output LENGTH, never its text, is annotated on the raised error); (2) `ModelAPIError.from_provider_response` accepts a plain error dict (the status-less in-stream case) and classifies by the documented error type (`overloaded_error` → SERVICE_UNAVAILABLE/retryable, `rate_limit_error` → RATE_LIMIT/retryable, auth/permission/invalid-request → their classes; unknown types keep UNKNOWN but the real provider message); (3) both Anthropic adapters now put the NORMALIZED token into `ResponseMetadata.finish_reason` (`max_tokens` → `length` — the contract split the schema documents and `openai.py` already implements; the raw token stays on `stop_reason`), and a truncation that produced zero output gets the cross-adapter placeholder content (openai convention) so callers never see `content=None`. Latent gap recorded, not fixed: a thinking-only response (thinking set, no text/tool calls, `end_turn`) would still fail the validator — unreachable while no caller enables extended thinking on these adapters.
 

docs/architecture/framework/decisions/ADR-010-realruntime-per-branch-instance-threading.md

@@ -0,0 +1,57 @@
+# ADR-010: `RealRuntime` threads the per-branch agent instance — no shared per-step state
+
+**Status**: Accepted
+**Date**: 2026-06-23
+**Implements**: Framework Session 16 — `docs/implementation/framework/sessions/v0.3.0/16-realruntime-current-instance-race.md`
+**Fixes**: Issue #40 (parallel-invoke validation cross-talk)
+**Related**: ADR-002 (centralized response validation — the validator this bug fed a wrong identity), ADR-005 (unified-barrier algorithm — defines the concurrent dispatch that surfaced the race); DP-004 (branch isolation)
+
+> **Numbering note:** the decisions directory already contains two `ADR-009-*` files (`-full-payload-llm-tracing`, `-specialized-agent-serialization`) — a pre-existing collision. ADR-010 is the next free integer; renumbering 009 is out of scope for this fix.
+
+## Context
+
+`RealRuntime` is TRUNK-CRITICAL (`docs/architecture/framework/overview.md` §3) and is constructed **once per `Orchestra.run()`**. The orchestrator dispatches `RealRuntime.step(branch)` for every runnable branch as a concurrent `asyncio.Task` on that single shared object (the documented concurrency model — "true parallelism for I/O-bound `runtime.step` calls"). Sync sections between awaits run atomically, but `step()` spans an `await` (the `StepExecutor.execute_step` LLM/tool call).
+
+`step()` stashed the per-branch agent instance on a shared attribute — `self._current_instance = instance` — *before* that await, and `_translate()` read it back *after* the await to feed `ValidationProcessor.validate_coordination_action(agent=...)`. Under `parallel_invoke` fan-out, a sibling branch's `step()` overwrote `self._current_instance` during the await, so a branch's coordination action was validated against a **different** agent's identity — and therefore against that other agent's outgoing topology edges. The validator keys its edge check off `agent.name → topology_graph.get_next_agents(agent.name)`, so the result was a fabricated `"Agent <X> cannot invoke: [...]"` failure attributed to the wrong agent.
+
+The bug was latent when fanned-out workers shared identical outgoing edges (cross-talk passed validation silently) and surfaced non-deterministically when edge sets differed. This is a direct violation of **DP-004 branch isolation** ("Each branch maintains its own memory, trace, metadata, and status; no cross-branch sharing or mutation is allowed") — the shared `_current_instance` *was* cross-branch mutation of per-step state.
+
+## Decision
+
+Thread the per-branch agent instance **explicitly as a parameter** and remove the shared attribute entirely:
+
+- `_translate(self, marsys_result, branch)` → `_translate(self, marsys_result, branch, instance)`.
+- The validator call inside `_translate` passes `agent=instance` (the parameter) instead of `agent=getattr(self, "_current_instance", None)`.
+- `self._current_instance = instance` is deleted from `step()`.
+
+After this change `RealRuntime` holds **zero per-step mutable state**: its instance attributes are all read-only configuration set at construction; every per-step value (`instance`, `branch`, `context`, `marsys_result`) is a local threaded as a parameter.
+
+This is non-additive (a private method signature changes; a private attribute is removed) and therefore TRUNK-CRITICAL — but the public surface is untouched: the `Runtime` Protocol requires only `step(branch)`, whose signature is unchanged, and `_translate` / `_current_instance` are both private with a single internal call site.
+
+### Forward constraint
+
+**Do not reintroduce per-step caching on the shared `RealRuntime`.** Per-branch state is threaded as a parameter, never stashed on `self`. The runtime is shared across concurrently-dispatched branches; any `self.<x> = <per-step value>` written before an `await` and read after it is a DP-004 violation and a latent cross-talk race. Run-lifetime configuration on `self` is fine; per-tick values are not.
+
+## Rationale
+
+The fix conforms the lone outlier to the file's own existing convention. `step()` already passes `instance` explicitly to `execute_step` (`agent=instance`) and to `_build_content_only_diagnostic(branch, instance)` — `_current_instance` was the single place a per-step value round-tripped through shared mutable state. Parameter-passing is the day-one shape: had the file been written with concurrent dispatch in mind, `_translate` would have taken `instance` like its siblings. No new mechanism is introduced; this is extend-and-unify, not add-parallel.
+
+## Alternatives considered
+
+### `contextvars.ContextVar` for the current instance
+Rejected. A `ContextVar` is task-local and would also be race-free, but it stands up a *parallel* propagation mechanism beside the explicit parameter-passing the file already uses everywhere. It is less explicit, heavier, and a worse fit — a future reader would ask why one per-step value travels by context while all others travel by parameter.
+
+### Construct a fresh `RealRuntime` per branch
+Rejected. Defeats the documented "one runtime per `Orchestra.run()`" design and is a far larger TRUNK-CRITICAL blast radius (touches the orchestrator's dispatch construction) for no benefit over parameter-passing.
+
+## Consequences
+
+- **No backward-compatibility concern.** The public `Runtime` Protocol and `step(branch)` signature are unchanged; no consumer depends on the private `_translate` signature or the `_current_instance` attribute.
+- **Correctness.** `parallel_invoke` fan-out to agents with heterogeneous outgoing edges no longer cross-talks; each branch is validated against its own identity regardless of interleaving. DP-004 compliance is restored for the step/translate path.
+- **Regression guard.** A deterministic, no-LLM test (`tests/coordination/orchestrator/test_real_runtime_parallel_race.py`) forces the racy interleave via `asyncio.gather` + an `execute_step` that yields control; it is RED on the pre-fix code and GREEN after. The issue-#40 live `parallel_invoke` reproducer remains manual verification only (provider key, timing-dependent) — not a committed CI test.
+
+## Approval
+
+This ADR requires framework-team approval before code is written (TRUNK-CRITICAL policy).
+
+- [x] Framework lead approval: rezaho (Reza Hosseini), 2026-06-23 (approved the TRUNK-CRITICAL touch in the session that produced this ADR)

src/marsys/coordination/execution/real_runtime.py

@@ -86,7 +86,6 @@ async def step(self, branch: OrchestratorBranch) -> OrchestratorStepResult:
                 kind="FAIL",
                 error=f"agent {branch.current_agent!r} not registered",
             )
-        self._current_instance = instance
 
         steering_threshold = getattr(
             self.execution_config, "content_only_steering_threshold",
@@ -150,7 +149,7 @@ async def step(self, branch: OrchestratorBranch) -> OrchestratorStepResult:
             logger.debug("could not snapshot agent memory for branch %s", branch.id)
 
         # 5. Translate the MARSYS StepResult into an orchestrator StepResult.
-        result = await self._translate(marsys_result, branch)
+        result = await self._translate(marsys_result, branch, instance)
         # Stamp step_span_id so the orchestrator can forward it as
         # ``parent_step_span_id`` for child branches spawned this tick.
         step_span_id = context.get("step_span_id")
@@ -159,7 +158,7 @@ async def step(self, branch: OrchestratorBranch) -> OrchestratorStepResult:
         return result
 
     async def _translate(
-        self, marsys_result: Any, branch: OrchestratorBranch
+        self, marsys_result: Any, branch: OrchestratorBranch, instance: Any
     ) -> OrchestratorStepResult:
         """Translate a MARSYS branches/types.StepResult into the
         orchestrator's StepResult shape.
@@ -206,7 +205,7 @@ async def _translate(
         validation = await self.validator.validate_coordination_action(
             action=coord_action,
             data=coord_data,
-            agent=getattr(self, "_current_instance", None),
+            agent=instance,
             branch=None,
             exec_state=exec_state,
         )

tests/coordination/orchestrator/test_content_only_hard_limit.py

@@ -56,7 +56,6 @@ def _make_runtime(topology_graph=None) -> RealRuntime:
         session_id="test-session",
         execution_config=None,
     )
-    runtime._current_instance = instance
     return runtime