fix(a2a): make local callback delivery automatic and safely optional

[mabry1985]

Problem and change

A local ORBIS → protoAgent delegation failed before execution because ORBIS always advertised a localhost callback that protoAgent's default callback policy rejects. Allowlisting localhost then exposed a second failure: the callback sender had no credential accepted by ORBIS.

ORBIS now generates a private, persistent callback token automatically and accepts the SDK's notification-token header only for POST /a2a/callback. This token cannot authorize general A2A calls, and the operator credential is never shared. Explicit third-party callback destinations keep their own configured tokens.

If a peer rejects optional callback registration before any task/event is received, the client retries once without push configuration, inside the original timeout and using the same message ID. Normal response delivery then works without changing the peer's network policy. General stream failures and outer timeouts never trigger a second dispatch, even before a first event (the server may already be executing). Only confirmed pre-execution push-registration rejection is retried. Durable task requery remains available when callbacks are unavailable.

Validation

• Live smoke against the running protoAgent passed: direct-token configuration, authenticated probe, ORBIS_CALLBACK_OK response, and callback accepted by an isolated guarded receiver.
• Real SDK sender → guarded callback receiver passes; missing/wrong tokens and use on other routes are rejected.
• Real SDK client → guarded A2A peer completes after localhost callback rejection; the agent executes exactly once.
• Tests cover token persistence/0600 permissions, explicit override, external callback isolation, retry limits, and no replay after task start.
• Full Python suite passed in a combined checkout containing the setup, callback, and warmup fixes (two optional-dependency skips).
• ruff check ., git diff --check, and bun run build passed.

No broad allowlist or open-auth setting is introduced. Native spoken interaction is not claimed as validated by these protocol tests.

[mabry1985]

@vera review

[protoreview]

QA panel review — WARN

code-review-structural · head ff3ca54ad2d2 · formal

Low-risk PR with three minor hygiene items, all confirmed by verification. The docstring in delegate_adapters.py is the one worth fixing before merge — it actively misleads readers about the no-resend contract that the code now enforces. Verification upgraded the timing-unsafe != in a2a_auth.py and the deprecated get_event_loop() in the test from nit to minor; no findings were refuted. No panel disagreement; all three items were independently confirmed at head ff3ca54. No gaps: all three findings were verified.

Findings

	Severity	Location	Finding	Verified
🟡	minor	agent/delegate_adapters.py:416	The _dispatch_inner docstring still claims 'Any stream error/timeout falls back to one synchronous message/send' but the code now only catches asyncio.TimeoutE…	confirmed
🟡	minor	a2a_auth.py:131	The X-API-Key comparison uses timing-unsafe != while the bearer token path two lines below uses hmac.compare_digest.	confirmed
🟡	minor	tests/test_a2a_outbound.py:33	Deprecated asyncio.get_event_loop() is used instead of asyncio.get_running_loop() inside an async test.	confirmed · nearby, not gating

findings JSON (machine-readable)

[
  {
    "file": "agent/delegate_adapters.py",
    "line": 416,
    "severity": "minor",
    "category": "conventions",
    "claim": "The _dispatch_inner docstring still claims 'Any stream error/timeout falls back to one synchronous message/send' but the code now only catches asyncio.TimeoutError and raises A2ADispatchTimeout.",
    "evidence": "Line ~415-418 at head: \"\"\"Reliable synchronous ``message/send`` by default; opt into streaming (progress narration) with ``A2A_STREAM=1`` or a ``progress_callback`` + a card that advertises ``streaming``. Any stream error/timeout falls back to one synchronous ``message/send``. (Moved verbatim from the old ``_dispatch_a2a``; see that history for the hang-guard rationale.)\"\"\" \u2014 while the except block now reads: `except asyncio.TimeoutError as exc: raise A2ADispatchTimeout(f\"{delegate.name}: no response within {bound:.0f}s; request was not resent\") from exc`",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "Read agent/delegate_adapters.py at head ff3ca54: the docstring (line ~415) still says 'Any stream error/timeout falls back to one synchronous message/send' but the only except clause in the streaming path is `except asyncio.TimeoutError` which raises A2ADispatchTimeout \u2014 no fallback to message/send exists anymore.",
    "line_original": 512,
    "line_corrected": true
  },
  {
    "file": "a2a_auth.py",
    "line": 131,
    "severity": "minor",
    "category": "security",
    "claim": "The X-API-Key comparison uses timing-unsafe `!=` while the bearer token path two lines below uses `hmac.compare_digest`.",
    "evidence": "Line ~121 at head: `if api_key and request.headers.get(\"x-api-key\") != api_key:` vs line ~131: `if not hmac.compare_digest(header[len(\"Bearer \") :], active):`",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "Read a2a_auth.py at head ff3ca54: the X-API-Key check uses `!=` (line ~121) while the bearer check uses `hmac.compare_digest` (line ~131). The inconsistency is real; `!=` on a secret is timing-unsafe.",
    "line_original": 121,
    "line_corrected": true
  },
  {
    "file": "tests/test_a2a_outbound.py",
    "line": 33,
    "severity": "minor",
    "category": "conventions",
    "claim": "Deprecated `asyncio.get_event_loop()` is used instead of `asyncio.get_running_loop()` inside an async test.",
    "evidence": "Lines ~33 and ~36 at head: `t0 = asyncio.get_event_loop().time()` and `assert asyncio.get_event_loop().time() - t0 < 5.0`",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "Read tests/test_a2a_outbound.py at head ff3ca54: two calls to `asyncio.get_event_loop()` inside an `@pytest.mark.asyncio` test. Since Python 3.10, `get_event_loop()` is deprecated in favour of `get_running_loop()` when a loop is already running (which it is under pytest-asyncio). \u2014 nearby: in code this PR did not change (outside its changed lines and the functions they sit in) \u2014 reported, not gated (#232)",
    "nearby": true
  }
]

---

1 structural finding(s) are nearby notes, not part of the verdict: they sit in code this PR did not change (outside its changed lines and the functions those lines are in). Worth a look; not a request for this PR.

• tests/test_a2a_outbound.py:38 (minor) — Deprecated asyncio.get_event_loop() is used instead of asyncio.get_running_loop() inside an async test.

[protoreview]

QA panel review — FAIL

code-review-structural · head ff3ca54ad2d2 · formal

The PR narrows the streaming error handler from a broad catch-and-fallback to a timeout-only catch that raises. The one thing to fix first is the removed fallback: non-timeout streaming errors (network drops, auth failures, SDK errors) now propagate unhandled even when no task has started and a retry would be safe. The stale docstring at line 416 is a minor follow-up. Verification confirmed both findings with no disagreement. No coverage gaps: the heavily-changed file (delegate_adapters.py) is well-covered by the panel.

Findings

	Severity	Location	Finding	Verified
🟠	major	agent/delegate_adapters.py:512	The fallback to synchronous message/send on streaming failure has been removed: the old code caught (A2ADispatchError, asyncio.TimeoutError, httpx.HTTPError) a…	confirmed
🟡	minor	agent/delegate_adapters.py:416	The _dispatch_inner docstring still claims 'Any stream error/timeout falls back to one synchronous message/send' but the code now only catches asyncio.TimeoutE…	⚠️ uncertain

findings JSON (machine-readable)

[
  {
    "file": "agent/delegate_adapters.py",
    "line": 512,
    "severity": "major",
    "category": "removed-behavior",
    "claim": "The fallback to synchronous message/send on streaming failure has been removed: the old code caught (A2ADispatchError, asyncio.TimeoutError, httpx.HTTPError) and fell back, but the new code only catches asyncio.TimeoutError, so any non-timeout streaming error (network drop, auth failure, SDK error) now propagates unhandled to the caller even when no task has started and a retry would be safe.",
    "evidence": "Old: `except (A2ADispatchError, asyncio.TimeoutError, httpx.HTTPError) as e:` followed by `logger.warning(f\"[delegates] {delegate.name} streaming failed/timed out ({e}); \" \"falling back to message/send\")`. New: `except asyncio.TimeoutError as exc:` followed by `raise A2ADispatchTimeout(f\"{delegate.name}: no response within {bound:.0f}s; request was not resent\") from exc`. The A2ADispatchError and httpx.HTTPError cases are no longer caught.",
    "verdict": "confirmed",
    "note": "Diff shows the old `except (A2ADispatchError, asyncio.TimeoutError, httpx.HTTPError)` block (which fell through to a synchronous `client.send(prefer_stream=False)`) replaced by `except asyncio.TimeoutError` that raises. At head line 512\u2013515 the only except clause is `asyncio.TimeoutError`; any other exception from the streaming `client.send` propagates uncaught past the fallback `client.send` at line 517."
  },
  {
    "file": "agent/delegate_adapters.py",
    "line": 416,
    "severity": "minor",
    "category": "conventions",
    "claim": "The _dispatch_inner docstring still claims 'Any stream error/timeout falls back to one synchronous message/send' but the code now only catches asyncio.TimeoutError and raises A2ADispatchTimeout \u2014 no fallback to message/send exists.",
    "evidence": "Line ~416 at head: \"\"\"Reliable synchronous ``message/send`` by default; opt into streaming (progress narration) with ``A2A_STREAM=1`` or a ``progress_callback`` + a card that advertises ``streaming``. Any stream error/timeout falls back to one synchronous ``message/send``. (Moved verbatim from the old ``_dispatch_a2a``; see that history for the hang-guard rationale.)\"\"\" \u2014 while the except block now reads: `except asyncio.TimeoutError as exc: raise A2ADispatchTimeout(f\"{delegate.name}: no response within {bound:.0f}s; request was not resent\") from exc`",
    "verdict": "uncertain",
    "note": "Read at head lines 416\u2013419: docstring verbatim says 'Any stream error/timeout falls back to one synchronous message/send'. The only except clause (line 512) catches asyncio.TimeoutError and raises \u2014 no fallback send exists. Stale docstring confirmed. \u2014 the verifier's note restates the claim and quotes no code \u2014 not evidence; downgraded to uncertain, cannot gate a merge (issue #259)",
    "evidence_restated": true
  }
]

---

1 finding(s) downgraded to uncertain: a confirmed must rest on code or tool output the claim did not already contain, and a claim about a library's behaviour on that library's own source, docs or a run of it (issue #259). Each still stands for a human to judge.

• agent/delegate_adapters.py (minor) — the verifier's note restates the claim and quotes no code

[mabry1985]

Review follow-up at 07dbfcd:

• Corrected both stale fallback docstrings, changed legacy API-key comparison to hmac.compare_digest, and replaced deprecated event-loop calls in the test.
• Expanded the no-resend regression to cover both task_started=False and True. A missing first event does not establish that no work started: the peer can accept work before the response is lost. Retrying a generic network/auth/SDK failure is therefore not a safe fallback. The error is handled by the caller; it is intentionally not converted into a new task.
• Explicit pre-execution optional-push rejection still retries once, with the same message ID and deadline. The real-SDK callback rejection test verifies exactly one agent execution. The SDK also negotiates synchronous transport for agents whose cards do not advertise streaming.

Both the focused callback/auth/outbound tests and the full Python suite pass; ruff and diff checks pass. Please re-review the latest head with the no-duplicate-execution contract above. Restoring the broad fallback would reintroduce duplicate work.

[protoreview]

QA panel review — FAIL

code-review-structural · head 07dbfcdc87aa · formal

Two findings survived verification, both confirmed. The most load-bearing change is the removal of the synchronous fallback in delegate_adapters.py — non-timeout streaming errors now propagate unhandled where they previously fell back to message/send. The panel disagreed on whether this was a defect or a deliberate design choice; the verifier confirmed it as a defect (the round-1 panel flagged it, and the refutation rule does not apply). The second finding (origin allowlist blocking non-browser clients) is pre-existing code, not introduced by this PR, but the behavioral claim is accurate. No gaps: both findings were fully verified.

Findings

	Severity	Location	Finding	Verified
🟠	major	agent/delegate_adapters.py:513	The fallback to synchronous message/send on streaming failure has been removed: the old code caught (A2ADispatchError, asyncio.TimeoutError, httpx.HTTPError) a…	confirmed
🟡	minor	a2a_auth.py:137	Origin allowlist blocks all non-browser clients when configured: when A2A_ALLOWED_ORIGINS is set, non-browser callers (other A2A agents, SDK-based tools, curl)…	⚠️ uncertain

findings JSON (machine-readable)

[
  {
    "file": "agent/delegate_adapters.py",
    "line": 513,
    "severity": "major",
    "category": "removed-behavior",
    "claim": "The fallback to synchronous message/send on streaming failure has been removed: the old code caught (A2ADispatchError, asyncio.TimeoutError, httpx.HTTPError) and fell back, but the new code only catches asyncio.TimeoutError, so any non-timeout streaming error (network drop, auth failure, SDK error) now propagates unhandled to the caller even when no task has started and a retry would be safe. Note: the correctness finder disputes this as a deliberate design choice (documented in the updated docstring, backed by new tests, and the task_started flag gives callers an informed decision point).",
    "evidence": "Old: `except (A2ADispatchError, asyncio.TimeoutError, httpx.HTTPError) as e:` followed by `logger.warning(f\"[delegates] {delegate.name} streaming failed/timed out ({e}); \" \"falling back to message/send\")`. New: `except asyncio.TimeoutError as exc:` followed by `raise A2ADispatchTimeout(f\"{delegate.name}: no response within {bound:.0f}s; request was not resent\") from exc`. The A2ADispatchError and httpx.HTTPError cases are no longer caught.",
    "verdict": "confirmed",
    "note": "Diff shows old `except (A2ADispatchError, asyncio.TimeoutError, httpx.HTTPError)` replaced with `except asyncio.TimeoutError`; head code at line 510 confirms only TimeoutError is caught. a2a_outbound.py wraps all send failures in A2ADispatchError, so non-timeout errors propagate unhandled. Round-1 panel flagged this as a defect (not a requested change), so the refutation rule does not apply.",
    "line_original": 512,
    "line_corrected": true
  },
  {
    "file": "a2a_auth.py",
    "line": 137,
    "severity": "minor",
    "category": "security",
    "claim": "Origin allowlist blocks all non-browser clients when configured: when A2A_ALLOWED_ORIGINS is set, non-browser callers (other A2A agents, SDK-based tools, curl) send no Origin header, so the check `if origin not in allowed` evaluates `\"\" not in [\"https://app.example.com\"]` \u2192 True, returning 403 to all non-browser clients.",
    "evidence": "When A2A_ALLOWED_ORIGINS is set, the middleware extracts the Origin header via request.headers.get(\"Origin\", \"\").lower(). For non-browser clients (other A2A agents, SDK-based tools, curl), the Origin header is absent, so origin becomes \"\". The check `if origin not in allowed` then evaluates `\"\" not in [\"https://app.example.com\"]` \u2192 True, returning 403.",
    "source": "protopatch",
    "verdict": "uncertain",
    "note": "Read a2a_auth.py at head lines 137-141: `origin = request.headers.get(\"Origin\", \"\").lower()` then `if origin not in allowed: return 403`. Empty string is not in any non-empty allowlist, so non-browser clients get 403. Pre-existing code (not in this PR's diff), but the behavioral claim is accurate. \u2014 evidence not found at the reviewed head \u2014 downgraded to uncertain, cannot gate a merge (issue #25)",
    "ungrounded": true
  }
]

---

1 finding(s) downgraded to uncertain: the code they quote as evidence does not appear in the file at the reviewed head, nor in this PR's patch for it. A finding that cannot be grounded does not gate a merge (issue #25) — it still stands for a human to judge.

• a2a_auth.py (minor) — quoted evidence not found at this head: "" not in ["https://app.example.com"]

---

Addressed since the last round (1 minor/nit): agent/delegate_adapters.py:416 — no longer raised, and the PR changed the code around them since they were raised.