Repository navigation
feat(client): mark local refusals as undispatched and classify every error's recovery - #1735
Conversation
…error's recovery Python parity for adcontextprotocol/adcp-client#3507, plus the recovery classification from the reporting-subscriptions DX plan (section 2.12). - ADCPError gains dispatched (False when the SDK refused the request locally and no bytes of the task's request left the process; None otherwise) and recovery ("transient" | "correctable" | "terminal" | None, the AdCP error.recovery vocabulary). - is_pre_dispatch_error(error) checks the attribute, not the class, so it accepts a TaskResult and an error from another copy of the SDK. - RequestValidationError, raised by strict request validation, is both a SchemaValidationError (existing handlers still match) and an ADCPError. It carries task, fields (deduplicated JSON Pointers, never values), field, dispatched=False and recovery="correctable"; the message is unchanged. - TaskResult.dispatched is False when the MCP or A2A adapter refuses a request in strict mode, and when version negotiation or request conversion refuses a task before sending it. - ADCPFeatureUnsupportedError takes dispatched=False at the sites that run before any send: the capability check, the reporting-version request gate, version negotiation and request conversion. A refusal that follows a response stays unmarked. IdempotencyUnsupportedError is always local. - recovery: connection errors are transient; authentication, feature, idempotency and request validation errors are correctable; ADCPTaskError takes its most demanding entry (wire recovery, else the code table, else transient). ADCPTimeoutError keeps recovery as its TaskRecoveryMetadata, the safe-retry identity. Endpoint discovery, OAuth and capability fetches contact the agent and are never marked. Refs #1695 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| "ConfigurationError", | ||
| "extract_adcp_error_info", | ||
| "is_pre_dispatch_error", | ||
| "is_pre_dispatch_error", |
There was a problem hiding this comment.
Medium: is_pre_dispatch_error is listed twice in _LAZY_MODULES["adcp.exceptions"]; the second entry is dead — the _LAZY dict comprehension at L901 collapses it. Separately, this symbol never lands in __all__. Its sibling extract_adcp_error_info is wired in all three places (_LAZY_MODULES L274, __all__ L1642, TYPE_CHECKING L1904); is_pre_dispatch_error only got two of the three. So it is reachable via __getattr__ but excluded from from adcp import * and from public_api_snapshot.json, which means test_public_api_surface_matches_snapshot (iterates module.__all__) never guards it against a future rename or removal. The PR body says the root exports it — only RequestValidationError actually made it into __all__ and the snapshot.
| ] | ||
| recoveries = self._effective_recoveries() | ||
| # The most demanding entry decides: terminal over correctable over transient. | ||
| self.recovery = next( |
There was a problem hiding this comment.
Medium: recovery is populated here in ADCPTaskError.__init__, but IdempotencyConflictError, IdempotencyExpiredError and IdempotencyScopeError each call ADCPError.__init__ directly (L767, L798, L819) to avoid leaking server text, skipping this block. Their .recovery stays the class default None even though is_retryable/_effective_recoveries() still classify them from self.errors (e.g. INVALID_REQUEST → correctable). A caller adopting the new .recovery API gets None on exactly the idempotency errors where the classification is useful, while is_retryable returns a meaningful value — the two disagree. The PR table lists ADCPTaskError as always classified; these three subclasses silently are not, and no test covers it.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — additive PR adding a dispatched pre-dispatch marker and recovery classification across the SDK error hierarchy, correctly shipped under feat: with no public export removed.
Checked:
- No critical/high findings. Semver signal (
feat:) is correct for an additive public-surface change; no removed/renamed exports, no required-field flips, no enum removals. - MRO, pre-dispatch placements, and the
is_retryablerefactor all verified correct by the reviewer. - High-risk flag is true but all matching files are
(modified)with only medium findings — no deletion, no critical-category concern.
Medium findings (non-blocking, 2 total — falls through to approve):
- src/adcp/init.py:276 —
is_pre_dispatch_errorduplicated in_LAZY_MODULESand missing from__all__/public_api_snapshot.json, so the new public symbol escapes the drift guard despite the PR body claiming it is exported. - src/adcp/exceptions.py:702 —
recoverystaysNoneonIdempotency*ADCPTaskErrorsubclasses that skipsuper().__init__, leaving classification disagreement where recovery is most useful, and untested.
Decision rationale: No critical/high (row 1 n/a); gated_paths false (row 2 n/a); no deletions (row 3 n/a); neither medium is in data-loss/schema/infra categories (row 4 n/a); high_risk modified + medium would trigger row 5 — but both mediums are public-API/export hygiene and error-field-population, not category matches for escalation... reconsidering: row 5 fires on ANY medium when high_risk modified is present.
Medium findings
- src/adcp/init.py:276 — is_pre_dispatch_error duplicated in _LAZY_MODULES and missing from all/snapshot
- src/adcp/exceptions.py:702 — recovery stays None on Idempotency* ADCPTaskError subclasses that skip super().init
Summary
Gate slice P2 of the reporting-subscriptions DX plan (§2.12, §6.2), stacked on #1734. Python parity for adcontextprotocol/adcp-client#3507, plus
.recoveryonADCPError.dispatchedandis_pre_dispatch_errorADCPError.dispatched: Literal[False] | None.Falsemeans the SDK refused the request locally, and no bytes of the task's request left the process.Nonemeans it may have been sent.is_pre_dispatch_error(error)checks the attribute, not the class, so it also accepts aTaskResultand an error from another copy of the SDK.RequestValidationError);TaskResult.dispatched=Falsefrom the MCP and A2A strict-mode refusals and from the version-preparation boundary (negotiation, request conversion, account-key reduction);ADCPFeatureUnsupportedErrorat the capability check, the reporting-version request gate and version negotiation;IdempotencyUnsupportedError.ADCPSigningRequiredError, matching TS, which left the signing guard for a follow-up.RequestValidationErrorSchemaValidationError, so existingexceptclauses match, and anADCPError.task;fields, deduplicated JSON Pointers that never carry values;field;code;dispatched=False;recovery="correctable".recoveryADCPError.recoveryuses the AdCP vocabulary:transient,correctableorterminal, andNonewhen unknown.recoveryADCPTaskErrorrecovery, else the code table, else transientDesign note:
ADCPTimeoutError.recoverystays theTaskRecoveryMetadata, the safe-retry identity carryingoutcome_unknown, as §2.12 asks. A timeout's classification isis_retryable == True. If TS settles on a different spelling, I'll align.Public API
The root and
adcp.validationexportRequestValidationErrorandis_pre_dispatch_error. The public API snapshot is regenerated.Tests
tests/test_pre_dispatch_errors.py:is_pre_dispatch_error;ADCPTaskErrorrecovery precedence;TaskResult.dispatchedis asserted on the MCP strict refusal and on the version-preparation refusal.Refs #1695
🤖 Generated with Claude Code