Skip to content

feat(a2a)!: checkpoint API + harden context-id retention#258

Merged
bokelley merged 1 commit intomainfrom
bokelley/pr-251-review
Apr 23, 2026
Merged

feat(a2a)!: checkpoint API + harden context-id retention#258
bokelley merged 1 commit intomainfrom
bokelley/pr-251-review

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

Summary

Follow-up to #251 addressing the post-merge audit from @scr-oath, plus additional hardening from a code-review / security-review pass.

What changed

Correctness

  • Ordering fix (a2a.py): _context_id and _active_task_id commit only after _process_task_response and the idempotency-error check both succeed. A raise from either previously left the adapter advanced to a response the caller never saw, potentially orphaning the in-flight task on retry.
  • Empty-string guard (client.py): context_id="" from os.getenv(...) or "" patterns is now treated as "not provided" instead of being seeded on the wire.
  • unknown TaskState: explicit — clears active_task_id and logs a warning.

API shape

  • Rename: pending_task_idactive_task_id (public on ADCPClient + A2AAdapter, and internal _pending_task_id). The old name read as "queued, not yet sent"; the real semantic is "server-side task the next call must echo to resume."
  • Checkpoint API:
    state: Checkpoint = client.checkpoint()
    # persist state (e.g. Redis)
    client = ADCPClient.from_checkpoint(agent_config, state)
    Checkpoint is a TypedDict carrying agent_id, context_id, active_task_id. Fixes the Redis-resume story for HITL flows — persisting only context_id would orphan the pending task. from_checkpoint rejects mis-matched agent_id (Agent A's checkpoint restored onto Agent B would leak session ids to a different vendor).
  • Exception type: ValueErrorTypeError on non-A2A context_id= / reset_context() / from_checkpoint(active_task_id=...). The string value is fine; the operation doesn't apply to non-A2A.

Internals

  • _NONTERMINAL_TASK_STATES: frozenset[TaskState] — coupled to the a2a.types.TaskState enum so an upstream rename becomes a type error, not a silent retention regression.
  • A2AAdapter._restore_active_task_id() — narrow internal setter used only by from_checkpoint so rename-safety fails loudly.

Coverage

  • Unit tests for every non-terminal retain (submitted, working, input-required, auth-required) and every terminal clear (completed, failed, canceled, rejected, unknown).
  • Two tests pinning the ordering invariant: one for the IdempotencyConflictError propagation path, one for the converted-to-TaskResult(FAILED) path.
  • Tests for checkpoint() round-trip, agent_id mismatch rejection, and non-A2A active_task_id rejection.

Breaking changes

  1. ADCPClient.pending_task_idADCPClient.active_task_id (same for A2AAdapter).
  2. ADCPClient(context_id=...) / reset_context() raise TypeError (was ValueError) on non-A2A.

Both pre-GA; callers catching ValueError or reading pending_task_id must update.

Test plan

  • ruff check src/ — clean
  • mypy src/adcp/ — 679 files, clean
  • pytest tests/test_protocols.py — 70/70 pass
  • pytest tests/integration/test_a2a_context_id.py — 5/5 pass (real uvicorn-hosted a2a-sdk Starlette app)
  • Full non-integration suite — 2069 passed, 21 skipped (baseline 2065 + 4 new state-transition tests)
  • Independent code-review + security-review passes; all SHOULD FIX items addressed

🤖 Generated with Claude Code

Follow-up to PR 251 addressing a post-merge audit. Highlights:

- **Ordering fix**: `_context_id` and `_active_task_id` now commit only
  after `_process_task_response` and the idempotency-error check both
  succeed. Previously, a raise from either would leave the adapter
  advanced to reflect a response the caller never received, potentially
  orphaning the in-flight task on retry.
- **Rename**: `pending_task_id` → `active_task_id`. The name now matches
  the semantic ("server-side task the next call must echo to resume").
- **Checkpoint API**: `ADCPClient.checkpoint()` returns a typed
  `Checkpoint` (TypedDict with `agent_id`, `context_id`,
  `active_task_id`). `ADCPClient.from_checkpoint(config, state)`
  rehydrates both ids. Fixes the advertised Redis-resume story for HITL
  flows — persisting only `context_id` would orphan the pending task.
  Restore validates `agent_id` against the target config so a
  checkpoint minted for Agent A can't leak session tokens to Agent B.
- **Enum coupling**: `_NONTERMINAL_TASK_STATES` is now
  `frozenset[TaskState]`, so an upstream rename in a2a-sdk becomes a
  type error instead of a silent retention regression.
- **`unknown` TaskState**: explicit — clears `active_task_id` and logs
  a warning so operators notice if a server starts emitting it.
- **Empty-string guard**: `context_id=""` from `os.getenv(...) or ""`
  patterns is now treated as "not provided" instead of being echoed on
  the wire.
- **Coverage gaps**: added unit tests for `submitted`/`auth-required`
  retention, `canceled`/`rejected`/`unknown` clearing, ordering
  invariant under both raised and converted-to-FAILED exception paths,
  and checkpoint mis-restore rejection.

BREAKING CHANGE: `ADCPClient.pending_task_id` is now
`ADCPClient.active_task_id` (same for `A2AAdapter`). The constructor's
`context_id=` kwarg and `reset_context()` now raise `TypeError` (was
`ValueError`) on non-A2A protocols — the string value is fine, the
operation doesn't apply to MCP.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bokelley bokelley force-pushed the bokelley/pr-251-review branch from 4f4c36c to a17ffb3 Compare April 23, 2026 11:07
@bokelley bokelley merged commit 14e7925 into main Apr 23, 2026
10 checks passed
@bokelley bokelley deleted the bokelley/pr-251-review branch April 23, 2026 11:19
bokelley added a commit that referenced this pull request Apr 23, 2026
…te enum

- Export ``Checkpoint`` TypedDict from the top-level ``adcp`` package
  so callers can ``from adcp import Checkpoint`` instead of reaching
  into ``adcp.client`` for the persistence-key type.
- Compare ``task.status.state`` against ``TaskState.completed`` /
  ``TaskState.failed`` in ``_process_task_response`` to match the
  enum-based membership check in ``_NONTERMINAL_TASK_STATES`` — an
  upstream rename in a2a-sdk now fails as a type error instead of a
  silent classification drift.

Also flips the in-progress release-please PR from 5.0.0 to 4.0.1. The
prior commit (#258) landed breaking-style changes (``pending_task_id``
→ ``active_task_id``, ``ValueError`` → ``TypeError`` on non-A2A) but
4.0.0 shipped under 48h earlier with no known A2A adopters — the
salesagent migration is on MCP, and A2A adoption is deferred behind
pluggable TaskStore / push-notification work. Treating those as
pre-adoption corrections rather than a major bump.

Release-As: 4.0.1

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant