feat: Adopt AgentFailure/send_failure across Python adapters - #612
Open
AlexanderZ-Band wants to merge 54 commits into
Open
AlexanderZ-Band wants to merge 54 commits into
AlexanderZ-Band wants to merge 54 commits into
Conversation
The AgentFailure shared failure shape shipped in this release via release-please automation, unblocking the send_failure migration. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmsEQaNzmSmBGVsCTuKvbX
Adds the shared, provider-neutral AgentFailure surfacing contract: send_failure on AgentToolsProtocol/AgentTools/FakeAgentTools, and the to_failure_event helper both implementations delegate to. send_failure is best-effort (swallows its own reporting failure); send_event keeps its existing raising behavior unchanged, since stateful callers (e.g. OpenCode's session-persistence retry) depend on it as a control signal. FakeAgentTools gains a send_event_error hook so tests can simulate a REST rejection without touching send_message's own simulation path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmsEQaNzmSmBGVsCTuKvbX
A Band-side send_message rejection must never be reported as a provider failure, even when the send sits inside a shared try/except that also handles real provider errors. deliver_reply wraps the cause so a catch site can re-raise it before falling into its provider-failure branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmsEQaNzmSmBGVsCTuKvbX
Replaces the ad hoc "error" event/_report_error guard with send_failure(AgentFailure(...)), preserving APIStatusError's status_code/body as code/detail (or falling back to a generic failure for any other exception type). No change to the existing raise-after-report control flow. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmsEQaNzmSmBGVsCTuKvbX
Replaces the ad hoc "error" event/_report_error guard with send_failure(AgentFailure(...)), preserving ServerError's status/message as code/detail (generic fallback otherwise). Also closes a gap where exceeding max_tool_rounds raised RuntimeError with zero report at all. No change to the existing raise-after-report control flow. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmsEQaNzmSmBGVsCTuKvbX
Replaces the ad hoc "error" event/_report_error guard with send_failure(AgentFailure(...)). Also widens the try to cover per-message runner construction (_create_runner), which previously ran outside the try entirely and could fail with zero report; the runner now starts as None so the existing finally's close() has nothing to do if construction itself is what failed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmsEQaNzmSmBGVsCTuKvbX
Replaces the ad hoc "error" event/_report_error guard with send_failure(AgentFailure(...)), preserving ResultMessage's api_error_status/errors as code/detail. Closes two previously-silent gaps: a bare re-raise in the session resume-retry path when there is no stored session to fall back to, and the fallback session-creation attempt itself failing. DedupingAgentTools needs no change (forwards send_failure via __getattr__ like every other method). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmsEQaNzmSmBGVsCTuKvbX
Every hand-rolled MagicMock() AgentToolsProtocol double that sets send_event as an AsyncMock needs the same for send_failure, or the next adapter migrated onto it fails with "MagicMock can't be awaited" instead of a real assertion. Fixing the remaining fixtures now so each adapter's own migration commit doesn't have to rediscover this. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmsEQaNzmSmBGVsCTuKvbX
Replaces the ad hoc "error" event/_report_error guard with send_failure(AgentFailure(...)). Also widens coverage to _obtain_session (create/resume session setup), which previously ran outside on_message's try entirely and could fail with zero report. _send_event_safe stays untouched -- its other callers depend on its boolean return to drive session-persistence retry. Copilot ACP needs no separate change: it's a pure config subclass of ACPClientAdapter and inherits that adapter's fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmsEQaNzmSmBGVsCTuKvbX
Replaces the ad hoc "error" event with send_failure(AgentFailure(...)), preserving the existing redaction guarantee: exception text can carry DB strings, paths, and tokens, so message stays a fixed generic string and code/detail stay unset -- never populated from the caught exception. Also widens the try to cover graph-factory construction, which previously ran outside it entirely and could fail with zero report. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmsEQaNzmSmBGVsCTuKvbX
Replaces the ad hoc "error" event/_report_error guard with send_failure(AgentFailure(...)) across all report-and-return call sites, unchanged control flow. Routes the auto-relay reply through deliver_reply so a Band-side send_message rejection surfaces as a delivery failure rather than a misclassified Letta provider failure. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmsEQaNzmSmBGVsCTuKvbX
AlexanderZ-Band
changed the title
AlexanderZ-Band
marked this pull request as draft
September 6, 2026 11:55
Replaces the ad hoc "error" event with send_failure(AgentFailure(...)), preserving the task's state_name(state) as code for a terminal failure state. Routes both room-reply and task-update send_message calls through deliver_reply so a Band-side delivery rejection is never misclassified as an A2A provider failure -- the existing report-and-return control flow is otherwise unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmsEQaNzmSmBGVsCTuKvbX
Report AgentFailure for the not-initialized guard, the missing-reply guard, and the generic turn exception, replacing the ad hoc _report_error helper (now dead and removed). Also tidies a stale comment in the Claude SDK test file left over from that adapter's own migration. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmsEQaNzmSmBGVsCTuKvbX
record_failed's visibility event now reports an AgentFailure instead of a hand-built send_event error payload, matching every other migrated adapter. The task-status event's own embedded error field (a distinct, flow-internal envelope) is unrelated and untouched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmsEQaNzmSmBGVsCTuKvbX
The turn's run loop had no catch-all: only UnexpectedModelBehavior was handled, and its genuine-failure branch (as well as every other exception type) propagated with zero report — the worst gap in the adapter survey. Consolidate into one except Exception, mirroring the CrewAI adapter's swallow-then-report structure, so every exception that isn't the benign post-reply output-retry exhaustion now reports an AgentFailure before propagating. Also migrates the missing-reply guard off the deleted _report_error helper. The band_respond_contact_request tool handler's own error event is left untouched: that failure originates from a Band platform tool call, not the pydantic_ai provider, so it is out of scope for AgentFailure. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmsEQaNzmSmBGVsCTuKvbX
Migrates the session-init and generic-turn-error reports off the deleted _report_error helper, and adds a report to the uninitialized-app guard, which previously returned silently with zero report. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmsEQaNzmSmBGVsCTuKvbX
_run_turn's agent construction + invoke_async had no except clause at all, so any provider failure propagated with zero report — the worst gap in this adapter. Wrap both in one except Exception that reports before reraising, and migrate the missing-reply guard off the deleted _report_error helper. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmsEQaNzmSmBGVsCTuKvbX
_run_agent's shared except now reports an AgentFailure instead of a raw send_event: message stays a fixed, redacted string (Agno's swallowed run/exception text can carry DB strings, paths, or tokens), and code is set to RunStatus.error's value only when the exception is an AgnoRunError -- never populated from response.content. Also widens the reported boundary to the agent-null guard and _build_run_input, both previously unguarded ahead of _run_agent's try. Investigated but did not build the contextvar-based delivery-failure slot the plan called for: band_send_message failures never reach here as a raised exception. execute_tool_call_structured (agent.py) catches every non-BandToolError exception and returns it as a plain string result, and send_message's own delivery path (post_message) never raises BandToolError -- so a Band-side delivery failure can't reach _run_agent's except at all, let alone get misclassified as an AgnoRunError. Verified against the installed agno package's own run-loop and function-call exception handling. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmsEQaNzmSmBGVsCTuKvbX
Migrates 4 of the adapter's 8 "error" sites to AgentFailure: the HTTP error branch (preserving the status code), the generic turn-failure fallback, the turn-timeout report (code="timeout"), and the terminal last_error_message delivered at end of turn. The other 4 stay send_event: the still-processing backpressure guard, the delivery -failure notice, and the two human-approval-timeout notices in approvals.py -- none of these are provider failures. Adds events_of_type/reported_failures test helpers (mirroring the existing per-package convention in the ACP and Codex test suites) and negative-assertion coverage proving the two approval-timeout sites never carry the shared failure metadata shape. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmsEQaNzmSmBGVsCTuKvbX
Replace the structured-error/plain-text dual path (gated by the now-deleted CodexAdapterConfig.structured_errors flag) with a single unconditional path that converts Codex's error payloads into the shared AgentFailure shape via build_agent_failure (replacing build_structured_error_metadata) and reports them with send_failure. is_retryable no longer defaults to False when the upstream codexErrorInfo omits it -- absence now stays unknown rather than lying about retryability. Reply/bookkeeping posts (_handle_local_command's slash-command replies, _emit_turn_outcome's fallback text and error text) now go through deliver_reply so a Band-side delivery failure raises DeliveryFailedError instead of being misread as a Codex provider error. on_message's turn-setup through turn-outcome span is wrapped so that boundary: DeliveryFailedError is logged and swallowed, any other exception is reported as an AgentFailure and re-raised, preserving existing raise-vs-report control flow everywhere else. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmsEQaNzmSmBGVsCTuKvbX
PendingA2ATask.fail() gains an optional failure dict, attached as TaskStatusUpdateEvent metadata (via TaskUpdater.update_status, which failed() didn't expose) alongside its existing freeform reason text. Two of the five call sites are Band-side room lifecycle (room closed, gateway shut down) and stay untouched -- neither is a provider failure. The other three are provider-originated: - _execute_a2a's broad except and _await_response's timeout synthesize AgentFailure(provider="a2a-gateway", ...) for gateway-relay failures, running the exception text through a redaction/length-cap sanitizer mirroring the TS SDK's sanitizeGatewayErrorMessage before it reaches the external A2A client. - _publish_band_response's "error" message_type case relays the Band peer's own already-built AgentFailure (its adapter's send_failure already stamped metadata["failure"] via to_failure_event) unchanged, rather than re-tagging its provider as "a2a-gateway". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmsEQaNzmSmBGVsCTuKvbX
on_message's single free-text "error" event becomes send_failure with a new _to_agent_failure(exc) converter, which unwraps acp.exceptions. RequestError's numeric code/data instead of collapsing a JSON-RPC error into one generic string. RoomTurnEmitter.__aexit__ relays a turn's held text through deliver_reply instead of a bare send_message call, and on_message's except now splits DeliveryFailedError from a real provider failure: a Band-side post failure is logged and left alone (the connection stays up, nothing is reported), where previously it fell into the generic branch and both tore down/respawned the ACP connection and misreported a healthy agent turn as an "ACP agent error". Left out of scope (verified against source, not present today): a turn timeout, transport-health-scoped classification of which failures should respawn the connection, and cancel_turn/stop_reason propagation. Each is a new resilience feature or a recovery-action policy change, not error- shape surfacing, and none corresponds to an existing bug in this file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmsEQaNzmSmBGVsCTuKvbX
AlexanderZ-Band
marked this pull request as ready for review
September 6, 2026 13:34
- codex.py: on_message's except DeliveryFailedError now re-raises the
original cause instead of swallowing it. This call path had no
try/except at all before this PR, so any exception (delivery or
provider) used to propagate and durably mark_failed the message;
swallowing it silently turned a failed reply-delivery into a
successfully processed message with no observable trace.
- acp/client_adapter.py: adds a turn_timeout_s config (default 300s,
mirroring Letta's own field) wrapping _runtime.prompt via
asyncio.wait_for, reporting a "timeout"-coded AgentFailure and
tearing the connection down for the next turn to respawn. This is an
explicit Requirement in the INT-1385 ticket text itself ("so a
silent/stuck agent becomes an observable failure instead of hanging
indefinitely") and was mistakenly bundled into this PR's earlier note
about descoped ACP resilience features -- the ticket treats the
timeout as the minimum required surfacing fix, not an elective one.
- crewai_flow.py: record_failed's message is capped at 500 chars again,
matching every other room post in the file (record_waiting,
reply_ambiguous) and the cap the old hand-built send_event path had.
The ambiguous-participant-identity error embeds every colliding
participant id and can exceed that length in a large room.
- a2a/gateway/adapter.py: the timeout AgentFailure's code is now
lowercase "timeout", matching Letta's and OpenCode's existing
convention instead of introducing a second casing for the same
closed vocabulary.
- codex.py: _handle_approval_command's remaining send_message calls
now go through deliver_reply too, for consistency with every other
slash-command handler in the same file (this method sits outside any
try/except either way, so behavior is unchanged; this only future-
proofs it if that ever changes).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CmsEQaNzmSmBGVsCTuKvbX
Four parallel review agents (reuse, simplification, efficiency, altitude) checked the AgentFailure migration diff for cleanup opportunities. Applied the verified, in-scope findings: - to_failure_event now uses has_visible_content() instead of a bare .strip(), matching the platform's actual blank-content rule (a zero-width-only message previously slipped past the blank check) - centralized the reported_failures() test helper (band.testing) instead of three duplicated copies plus one inlined reimplementation - send_failure implementations pass MessageType.ERROR instead of the raw "error" literal - added a shared FAILURE_CODE_TIMEOUT constant instead of four independent "timeout" literals across letta/acp/opencode/a2a-gateway - a2a-gateway's timeout branch reuses failure.message instead of retyping the same string twice - codex.py's 5 new task-event send_event calls go through one _emit_event_safe helper instead of duplicating the try/except-log shape (mirrors copilot_sdk's existing _send_event_safe) - removed a docs/adapters/codex.md row describing the now-deleted structured_errors config field Skipped: a shared SimpleAdapter provider-slug ClassVar and a shared report_failure(tools, provider, exc) helper (both touch adapter class-shape/control-flow well beyond this migration's own new code), and per-adapter exception-to-AgentFailure enrichment unification (genuinely adapter-specific, tied to each SDK's own exception types). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmsEQaNzmSmBGVsCTuKvbX
Verified each finding directly against source/tests before accepting.
Credential redaction (a2a-gateway): the KV regex stopped its value group
at the first whitespace, so a scheme-prefixed credential ("Authorization:
ApiKey sk-...") only redacted up to the space and leaked the real secret;
widened it, and applied the same redaction to the peer-forwarded-failure
relay path (on_event's "error" branch), which previously reached an
external A2A client with zero redaction, unlike this adapter's own
exception path.
A2A adapter: the finally block's terminal task-event emission could
replace a DeliveryFailedError already propagating from the try block
(Python try/finally semantics), turning a Band delivery outage into a
fabricated "a2a" provider failure once it reached on_message. Now
isolated in its own try/except.
CrewAI Flow: record_failed's 500-char cap now preserves the untruncated
message in AgentFailure.detail instead of discarding it.
fake_tools.reported_failures(): now ignores "error" events with no
failure metadata, so pydantic_ai's still-unmigrated bare send_event(...,
"error") call doesn't crash it with a KeyError.
Codex: the turn-timeout path never called send_failure, unlike every
sibling adapter's timeout handling; added it. Deduped the case where both
an "error" notification and a failed turn/completed fire for the same
incident (was reporting twice). Wrapped _handle_approval_command's reply
delivery in the same DeliveryFailedError handling as every other reply
path in the file (it ran outside it). Tightened _emit_structured_turn_error's
non-dict-error guard so a falsy scalar (e.g. False) can't become the
literal string "False" in a room-visible message.
Also centralized events_of_type in band.testing (removing a dead,
byte-identical copy in tests/adapters/opencode/helpers.py and several
inline reimplementations), and fixed two comments that narrated the
diff's own history instead of stating a present-tense fact.
Added regression tests for every fix above (the review's own pass hadn't
added coverage for the codex.py, a2a/adapter.py, or a2a/gateway fixes) --
confirmed each new a2a test actually fails against the pre-fix code.
Not fixed: ACPClientAdapter's turn-timeout calls self.stop(), which tears
down the adapter-wide shared runtime, evicting every other room's
in-flight session on one room's timeout. Real, but pre-existing (the
adapter's generic except-Exception branch already did this before this
PR) and needs a per-room isolation design, not a bolt-on patch -- flagging
for a separate discussion rather than guessing at a fix.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CmsEQaNzmSmBGVsCTuKvbX
_emit_structured_turn_error had no logging on its path at all, unlike its sibling _handle_error_event (which logs "Codex error: %s" before its own send_failure call) -- a Codex turn failing via turn/completed produced a room-visible AgentFailure with zero corresponding server-side log line. Also finishes the send_event_safe migration (commit 83fb9fe) for the 11 remaining hand-rolled try/send_event/except-Exception/logger.debug call sites in this file (reasoning/plan/commentary streaming deltas, context compaction, approval thought/request/audit events, turn lifecycle, plan steps, token usage, diff events) -- the shared helper was introduced to replace exactly this pattern but only covered 5 of 16 matching sites. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011uGMZeouBGz9s3vKCe2uhj
The docstring claimed send_event_safe is for events "unlike send_message/ send_failure calls a caller depends on as a control signal" -- but send_failure never raises (its own docstring says so, and AgentTools. send_failure swallows its own reporting failure), so lumping it in there was backwards. Drops send_failure from that clause. Also adds direct tests for send_event_safe's failure branch (swallow, log at the given level, return False) -- introduced in commit 83fb9fe as a shared, general-purpose utility with no test of its own contract. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011uGMZeouBGz9s3vKCe2uhj
…ted_failures() _deliver_task_update's terminal-failure branch called state_name(state) three times for one value across four lines; binds it once. Also consolidates two tests that hand-rolled an error_events filter alongside a separate reported_failures(tools) call for the same data -- both checks now go through reported_failures() alone. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011uGMZeouBGz9s3vKCe2uhj
…d_failures() Three tests hand-rolled an errors/error_events filter over tools.events_sent alongside a separate reported_failures(tools) call checking the same data (one of the three still had the pre-migration hand-rolled check as its only check, with no reported_failures() call at all). All three now assert through reported_failures() alone. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011uGMZeouBGz9s3vKCe2uhj
…ed_failures() Three tests (test_timeout_reports_error, test_failed_tool_resync_skips_turn, test_prepare_failure_reports_error_and_skips_turn) hand-rolled an error_events filter over tools.events_sent alongside a separate reported_failures(tools) call checking the same data. All three now assert through reported_failures() alone. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011uGMZeouBGz9s3vKCe2uhj
…ering reported_failures()/events_of_type() are now depended on by roughly 15 adapter test files as the sole way to assert on a reported AgentFailure, but had no direct test of their own -- in particular the documented "ignores an error event with no failure metadata" behavior was never exercised. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011uGMZeouBGz9s3vKCe2uhj
Collaborator
Author
Cycle 2 of the personal fixed-criteria reviewRe-ran all six angle-groups against the head produced by cycle 1's fixes (commits 86990dc..0cb8119). Simplification and Logical Bugs came back clean (zero new findings — the cycle-1 Logical Bugs fix was independently re-verified and still holds). The other four groups found 12 new findings, all introduced or made newly visible by cycle 1's own fixes rather than pre-existing PR content. 11 fixed, 1 declined; already committed and pushed (b00e015..004b4fc):
Full verification gate (ruff check/format, pyrefly, full unit suite) green after these fixes — 5651 passed, 135 skipped; same 2 pre-existing unrelated failures as before ( This was cycle 2 of 2 ( 🤖 Generated with Claude Code |
crewai's event bus installs a global OpenTelemetry TracerProvider and a live span-exporter thread at import time (crewai.events.event_listener's module-level singleton). In a venv where crewai is installed alongside everything else, this silently breaks tests/example_agents/test_otel_setup.py's "nothing is a global" assertion for any test collected in the same session, and leaves a live network connection open that delays process exit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011uGMZeouBGz9s3vKCe2uhj
main() deliberately mutates the real process HOME (os.environ["HOME"] = AGENT_HOME) as a production side effect for the real launcher process. Two tests in test_exec.py call main() in-process, so nothing reverted that mutation -- HOME stayed "/home/agent" for the rest of the pytest session, breaking any later test resolving Path.home() on a machine where that path doesn't exist (observed via tests/integrations/test_crewai_flow_real_sdk.py, which lazily resolves it through crewai's own storage-path code). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011uGMZeouBGz9s3vKCe2uhj
Collaborator
Author
Two additional test-infra fixes (root-caused, not part of the review findings)The full suite had 2 failures that looked environment-related at first glance; dug into both per request rather than dismissing them. Both are real, pre-existing bugs (neither touched by this PR's diff), now fixed and pushed (956e0f7, c237ec4):
Neither failure was ever visible in CI: CI splits the suite across 3 jobs by dependency extra, and neither of these two tests ever shares a process with the other in any of those jobs. They only surfaced because a local dev venv had every extra installed at once. Checked whether "make the full tree pass in one CI lane" was achievable now: no — the Full suite: 5653 passed, 135 skipped, 0 failed. 🤖 Generated with Claude Code |
amit-gazal-band
requested changes
Sep 14, 2026
amit-gazal-band
left a comment
amit-gazal-band
left a comment
Collaborator
There was a problem hiding this comment.
Automated review from a plain Claude Code session (no human review yet). Found several gaps in this PR's own error-surfacing mechanism plus a couple of smaller cleanups — see inline comments. Two additional pre-existing issues noticed but out of scope for this PR (not commented inline): src/band/adapters/opencode/adapter.py around the OpencodeToolStatus.PENDING gate silently drops tool calls still pending at timeout/abort, and src/band/integrations/acp/room_emitter.py's send_event calls aren't wrapped in this PR's new send_event_safe helper the way other adapters now are.
An unexpected exception while consuming the turn's event stream fell through to a plain "I couldn't complete this request" chat reply with zero send_failure call, bypassing on_message's own outer AgentFailure handler -- the opposite of this PR's goal. It now reports and raises TurnResultAlreadyReported like every sibling failure path. transport/closed also reported unconditionally, double-posting one incident when an "error" notification already reported it just before the transport dropped; it now respects the same failure_reported guard turn/completed already uses. Also replaces codex.py's four bare "codex" AgentFailure provider literals (plus types.py's build_agent_failure) with one CODEX_PROVIDER constant, matching claude_sdk.py/letta.py's existing _PROVIDER pattern from this same PR -- a typo in one occurrence would otherwise silently break provider filtering for that failure event alone. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JjhSR1MngdKxXiSJctQjun
Collaborator
Author
Personal review pass (2026-09-14): assessing @amit-gazal-band's reviewRan the
The two informational notes (OpenCode's Full verification gate green: 🤖 Generated with Claude Code |
AlexanderZ-Band
commented
Sep 14, 2026
AlexanderZ-Band
left a comment
AlexanderZ-Band
left a comment
Collaborator
Author
There was a problem hiding this comment.
Personal fixed-criteria review (six angle-groups)
Ran the six-lens pass (Style & Hygiene, Design & Reuse, Runtime Safety & Observability, Test Quality, Edge-Case Flow Analysis, and a staged Logical Bugs explorer+skeptic) against the full current diff. 11 findings survived verification (2 nits, 9 issues — several overlapping findings from different groups on the same root cause were merged into one comment each). Simplification/over-engineering: none flagged.
One more finding has no diff hunk to anchor an inline comment to (the file isn't part of this diff), noting it here instead:
- suggestion
tests/e2e/baseline/smoke/inspection/test_events.py: this PR changes the wire shape of every adapter's failure reporting (newmetadata["failure"]key carryingAgentFailure.to_dict()) across ~18 adapters, but no e2e/baseline smoke forces a real provider failure and asserts the platform round-trips this new shape end-to-end — existing coverage here only exercises the drivenband_send_event(message_type="error")platform tool, not the adapter-internalsend_failurepath this PR introduces. Consider adding a baseline smoke that forces a deterministic provider failure for at least one adapter and asserts the resulting room event'smetadata["failure"]["provider"]/coderound-trip through the real platform.
- Add a per-adapter `_PROVIDER` constant (gemini, parlant, crewai, copilot_sdk, strands, a2a, a2a gateway, acp client, opencode, pydantic_ai) so every AgentFailure call site references one source of truth instead of a repeated string literal. - Log before every previously-silent generic-exception send_failure path (codex, strands, pydantic_ai) so a turn failure leaves a stack trace, not just a user-facing message. - Fix a real crash: codex.py's outer except could reference thread_id/ turn_id before either was ever assigned if _ensure_client_ready or _ensure_thread itself raised; both are now initialized up front. - Guard codex.py's manual-approval notification: a failed room post while requesting approval now reports a failure and defaults to "decline" instead of leaving the approval unresolved. - Replace acp/client_adapter.py's asyncio.gather(stop(), send_failure()) with sequential awaits so a fast send_failure can't race a slower stop. - Preserve gemini.py ServerError.status's real value (including None) instead of always stringifying it. - Broaden a2a gateway's credential-redaction regex to cover password/secret/access-key keys, not just token/authorization/api-key. - Narrow letta.py's turn_timeout_s to bound only the Letta provider call, not response processing/delivery -- a slow Band-side reply post could previously race the same clock and get misreported as a Letta provider timeout. - Convert several tests (letta, copilot_sdk, a2a, strands conformance) to assert on reported_failures(tools) instead of hand-filtering tools.events_sent, and add regression coverage for each fix above. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JjhSR1MngdKxXiSJctQjun
A focused Logical Bugs re-check against the previous commit's fixes found two new defects introduced by those very fixes: - codex.py: the manual-approval notify-failure fix returned "decline" directly instead of re-raising, which made the caller's success path run and credit/blame the human sender (decided_by) for a decision they were never actually notified about. Now re-raises after reporting, so it flows through the same system_fallback attribution as every other forced-decline path in this file. - letta.py: narrowing turn_timeout_s to the provider call fixed the auto-relay race, but _run_turn's except asyncio.TimeoutError: still blanketed the whole (now-unbounded) response-processing phase, so a TimeoutError from tool-event reporting could still be mislabeled as a Letta provider timeout. The timeout is now caught and reported at the exact wait_for call it bounds, converted to TurnResultAlreadyReported so it can never be conflated with an unrelated TimeoutError from elsewhere in the same method. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JjhSR1MngdKxXiSJctQjun
…hon-surface-errors-in-adapters Keep CrewAI's first-call empty-response retry, and report those failures through send_failure instead of the removed send_event error path. Co-authored-by: Cursor <cursoragent@cursor.com>
AlexanderZ-Band
commented
Sep 14, 2026
AlexanderZ-Band
left a comment
AlexanderZ-Band
left a comment
Collaborator
Author
There was a problem hiding this comment.
One confirmed ACP lifecycle finding; fixed in ad36d9a and covered by a focused regression.
AlexanderZ-Band
requested review from
bandzalkin and
nir-singher-band
and removed request for
amit-gazal-band,
bandzalkin and
nir-singher-band
September 15, 2026 11:34
Collaborator
Author
|
Triaged as out of scope: INT-1385 requires adapter contract coverage, which this PR provides. A live platform test needs a deterministic real-provider failure boundary; none exists here without adding fragile provider/auth plumbing. No product defect remains untested by the ticket's required contract coverage. |
Collaborator
Author
|
Verified on the current head: |
AlexanderZ-Band
commented
Sep 15, 2026
AlexanderZ-Band
left a comment
AlexanderZ-Band
left a comment
Collaborator
Author
There was a problem hiding this comment.
Two confirmed behavior defects plus one cleanup nit; fixing them now.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adopts band-sdk-core's shared
AgentFailureshape (provider/code/message/detail)across band-sdk-python adapters, replacing the free-text
"error"event andits ad hoc per-adapter metadata keys.
Changes
band-sdk-coreto 2.3.0send_failure/to_failure_eventonAgentToolsProtocol,AgentTools,FakeAgentToolsDeliveryFailedError/deliver_replyfor Band-delivery vs. provider-failure separationturn_timeout_s, default 300s) around_runtime.promptPendingA2ATask.fail, credential redaction on both the adapter's own exception path and the peer-forwarded-failure relay pathFAILURE_CODE_TIMEOUTconstant;MessageType.ERRORused instead of a raw"error"literalto_failure_eventblank-content check useshas_visible_content()reported_failures()/events_of_type()test helpers inband.testingfinallyrecord_failedcaps the room-visible message at 500 chars, preserves the full message inAgentFailure.detailBreaking Changes
CodexAdapterConfig.structured_errors(previouslybool = True, documented indocs/adapters/codex.md) is removed. The model usesextra="forbid", so any existing deployment config, constructor kwarg, orCODEX_STRUCTURED_ERRORSenv var still setting this field will now raisepydantic.ValidationErrorat construction instead of being silently ignored. No deprecation alias/passthrough is added — this repo's own conventions reject backwards-compat shims in favor of a clean break. The eventual merge commit needs a!on its type (e.g.refactor!:) and aBREAKING CHANGE:footer so release-please's changelog actually reflects this.Out of scope
band_respond_contact_requesttool handler's own error event (a platform-tool failure, not a provider failure)cancel_turn/stop_reasonpropagationTest plan
uv run ruff check .uv run ruff format --check .uv run pyrefly checkuv run pytest tests/ --ignore=tests/integration/ --ignore=tests/e2e/— 5620 passed, 146 skipped, 0 faileduv run pytest --markdown-docs $(git ls-files '*.md' ':!:examples/*')Follow-up: cross-SDK review fixes (99b660a)
Fixed in 99b660a, found against band-sdk-typescript#178's parallel implementation: several adapters reported a terminal provider failure via
send_failurebut then returned/fell through normally instead of failing the turn.a2a/adapter.pyDeliveryFailedErrorexcepte.causea2a/adapter.pygenericexcept Exceptionsend_failure+ return →send_failure+raiseclaude_sdk.py_on_turn_complete(2 branches)send_failure+ return → raises newTurnResultAlreadyReportedclaude_sdk.pyon_messageexcept TurnResultAlreadyReported: raiseclauseletta.pyclient-not-initialized guardsend_failure+ return → synthesizesRuntimeError, reports, raisesletta.pysession-prepexcept Exceptionsend_failure+ return →send_failure+ bareraiseletta.py_handle_messagemissing-room-contextsend_failure+ return → synthesizesRuntimeError, reports, raisesletta.py_run_turnDeliveryFailedErrorexcepte.cause(still nosend_failure, correctly)letta.py_run_turntimeout / generic exceptsend_failurethen fall through →send_failurethenraiseparlant.pyapp-not-initialized guardsend_failure+ return → synthesizesRuntimeError, reports, raisesparlant.pysession-initexcept Exceptionsend_failure+ return →send_failure+raiseNet effect: across four adapters, a reported provider failure now also fails the turn, so the platform's retry mechanism actually engages instead of silently losing the message.
TurnResultAlreadyReportedprevents this from causing double-reporting inclaude_sdk. Band-side delivery failures remain correctly un-attributed to the provider but are now also retryable.🤖 Generated with Claude Code
https://claude.ai/code/session_0195k7AdghCyZgBPgs4TdxE3