From 2ab36de1518bd3d0ff38a80396502f5530adb356 Mon Sep 17 00:00:00 2001 From: andy Date: Tue, 3 Nov 2026 00:13:03 +0800 Subject: [PATCH 1/2] docs(agents): add task-state-invariants skill Most task-lifecycle defects in this SDK are not wrong values: they are a correct value that reached one surface and not the others (persisted store, streaming subscribers, push notifications, the cluster task_events table, and the two separate v0.3 compat mappers). The existing rule layer routes mistakes to mistake-reflection and code style to docs/ai, but nothing tells a change agent which surfaces a status write must reach, so the same class keeps recurring: #1276 (status.timestamp mapped for JSON-RPC/REST compat but not gRPC), #1188 (handler resumes from a stale snapshot), #1206/#1411 (contextId inference disagrees between events and the stored task), and the review findings on #1346 (stamping the copy instead of the event, and missing the CANCELED paths). Adds .agents/skills/task-state-invariants/SKILL.md with the invariant set (each claim anchored to current main), the sibling-surface checklist, the matching test files per surface, and the evidence expected in a PR, plus a one-line router pointer in AGENTS.md so agents actually load it. Signed-off-by: andy --- .agents/skills/task-state-invariants/SKILL.md | 145 ++++++++++++++++++ AGENTS.md | 7 + 2 files changed, 152 insertions(+) create mode 100644 .agents/skills/task-state-invariants/SKILL.md diff --git a/.agents/skills/task-state-invariants/SKILL.md b/.agents/skills/task-state-invariants/SKILL.md new file mode 100644 index 000000000..9a9a65ecb --- /dev/null +++ b/.agents/skills/task-state-invariants/SKILL.md @@ -0,0 +1,145 @@ +--- +name: task-state-invariants +description: Use when adding or changing code that writes A2A task state (Task, TaskStatus, TaskStatusUpdateEvent, TaskUpdater, task stores, push notifications, cluster event streams, v0.3 compat mappers), or when reviewing such a change — runs the sibling-surface propagation checklist so a state change or field lands on every surface (persisted store, streaming subscribers, push notification, event table, both compat transports) instead of only some of them. Not for style or type rules (read docs/ai/coding_conventions.md) and not the pre-commit gate (read docs/ai/mandatory_checks.md). +--- + +# Task State Invariants + +Implements the missing verification step for the task-lifecycle subsystem: +`AGENTS.md` tells you to run the check sequence, and `evidence_rules.md` +tells you to cite behavior, but neither tells you **which surfaces a task +state change must reach**. Most bugs in this area are not wrong values — +they are a correct value that reached one surface and not the others. + +## When to load this skill + +Trigger on ANY of these, without waiting for the user to ask: + +- You write or mutate `TaskStatus`, `Task`, `TaskStatusUpdateEvent`, or + `TaskArtifactUpdateEvent`. +- You touch `TaskManager.save_task_event`, a `TaskStore`, a + `VersionedTaskStore`, or `DatabaseTaskStore` column mapping. +- You touch push notifications, the event queue/consumer, or the cluster + event stream (`task_events`). +- You add or rename a protocol field, or touch `compat/v0_3/`. +- You transition a task to a terminal or interrupted state anywhere. +- A reviewer comment says some surface "still gets the old value", "keeps + the stale timestamp", or "is missing on the other transport". + +If unsure whether this covers your change: load it. Checking the matrix is +cheap; a half-propagated state ships a bug. + +## The invariant set + +Each claim is anchored to current `main`. Re-read the file before quoting. + +1. **A terminal state must carry `status.timestamp`.** + `TERMINAL_TASK_STATES` is defined once in `src/a2a/utils/constants.py:35` + and imported everywhere else — never re-declare a local copy. + `DatabaseTaskStore` derives `last_updated` from it + (`src/a2a/server/tasks/database_task_store.py:152-157`), so an unstamped + terminal state persists `NULL` and silently drops the task from + `ListTasks` ordering and `status_timestamp_after` filtering. +2. **Stamp the event, then copy — not the copy.** + `TaskManager.save_task_event` replaces status wholesale with + `task.status.CopyFrom(event.status)` + (`src/a2a/server/tasks/task_manager.py:285`). That same event object is + handed to streaming subscribers, push notifications, and the event table. + Stamping the local `task` after the copy leaves the event unstamped, so + the store is fixed and the other three surfaces stay broken. +3. **`CopyFrom` inherits the previous timestamp.** A copy-then-set-state + transition (for example the cancel path, + `src/a2a/server/agent_execution/active_task.py:971-973`) reuses whatever + timestamp the prior state had, so the terminal write looks like a + `WORKING` update. Set `status.timestamp` explicitly on every transition + you introduce. +4. **Push delivery is best-effort and never task-affecting.** + `_dispatch_notification` already contains delivery failures and returns + a bool; `send_notification` is awaited inline from the consumer at + `src/a2a/server/agent_execution/active_task.py:409`. Anything that can + raise there (config-store reads, URL validators, custom senders) must not + escape into the failure path, or a webhook hiccup rewrites a completed + task to `FAILED`. +5. **Every request must produce at least one event.** With zero events, + `on_message_send` falls through to `active_task.get_task()` + (`src/a2a/server/request_handlers/default_request_handler_v2.py:518-520`), + which awaits `_task_created` with no timeout + (`src/a2a/server/agent_execution/active_task.py:1115-1117`) and hangs the + request, producer, and consumer forever. Guard before awaiting, or the + fallback must be bounded. +6. **Store and stream must agree.** A blocking call may return a shared + snapshot while later events land on the store — the copy-on-yield caveat + is documented in-code at + `src/a2a/server/agent_execution/active_task.py:965-970` (tracked in + #1175 / #1191). After any change here, assert `GetTask` equals what the + caller received. +7. **v0.3 compat has two independent mappers.** `compat/v0_3/proto_utils.py` + serves the gRPC surface and `compat/v0_3/conversions.py` serves JSON-RPC + and REST. A field mapped in one and not the other appears and disappears + by transport — see #1276 (status timestamp dropped on gRPC only) and + #1403. Any compat field change must touch and test both. +8. **Continuation identity is inferred, not invented.** With only + `message.task_id`, the SDK must reuse the task's `context_id` + (A2A v1.0 §3.4.3); generating a fresh one makes events disagree with the + stored task — see #1206 and the open follow-up #1411. + +## Procedure + +1. Name the surfaces your change writes. Default assumption: **all five** — + persisted store, streaming subscribers, push notification, cluster event + table, v0.3 compat (both mappers). +2. Grep for the sibling call sites before editing: + + ```bash + grep -rn "save_task_event\|CopyFrom\|send_notification\|enqueue_event" src/a2a + ``` + +3. For each surface you touch, add or update a test in the matching file: + `tests/server/tasks/test_task_manager.py`, + `tests/server/tasks/test_database_task_store.py`, + `tests/server/agent_execution/test_active_task.py`, + `tests/server/tasks/test_push_notification_sender.py`, + `tests/compat/v0_3/test_proto_utils.py`, + `tests/compat/v0_3/test_conversions.py`. +4. Assert the **event object** as well as the stored task — not just the + store — for any timestamp or field you propagate. +5. If you added a field, state in the PR which transports carry it (REST, + JSON-RPC, gRPC, v0.3 compat each). +6. Then run the normal gate: `docs/ai/mandatory_checks.md`. + +## Evidence for the PR + +Per `docs/ai/evidence_rules.md`, a behavior claim needs control-flow +evidence. For this subsystem, the minimum is: + +- the surface list from step 1, with the file:line of each write; +- a test per surface touched, named for the surface; +- before/after output showing the surface that used to get the old value. + +## Anti-patterns + +- Fixing the store and assuming the stream follows. It does not — the event + is published separately. +- Re-declaring `TERMINAL_TASK_STATES` locally instead of importing it from + `a2a.utils.constants`. +- Testing compat through one transport and concluding the field works. +- Silently returning `None` from an executor: no event means no task, and + the caller blocks (invariant 5). +- Letting an infrastructure exception (store read, URL validation) escape + into the consumer failure path (invariant 4). + +## Boundaries + +- This skill does not replace `mandatory_checks.md`; it runs before that + gate, not instead of it. +- It does not decide scope. If a surface is intentionally out of scope, + say so in the PR and link the issue that owns it. +- If you find an invariant here violated in code, load + `mistake-reflection` only if *you* introduced the error; otherwise report + it as an issue with reproduction. + +## Provenance + +Derived from recurring defects in this subsystem and the review feedback on +them: #1313/#1329 (invariant 4), #1330 (5), #1331 (1, 2, 3), #1188 (6), +#1276 (7), #1206/#1411 (8), and community questions #248, #435, #40, #1224. diff --git a/AGENTS.md b/AGENTS.md index a4fbea5b2..28fbcea8b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -34,6 +34,13 @@ You MUST do all of the following: When unsure: load the skill. False positives are free; false negatives are how the same mistake recurs. +4. **When changing task state** (or reviewing such a change), load the + `task-state-invariants` skill at + `.agents/skills/task-state-invariants/SKILL.md` before editing. It lists + the sibling surfaces a status change must reach (persisted store, + streaming subscribers, push notification, cluster event table, both v0.3 + compat mappers) so a correct value does not land on some of them only. + ## Optional extras From bac33803afee6ee7060178cac54c19868bc3db54 Mon Sep 17 00:00:00 2001 From: andy Date: Tue, 3 Nov 2026 00:17:43 +0800 Subject: [PATCH 2/2] docs(agents): sharpen the compat-mapper citation #1403 is an error-handling divergence on the v0.3 REST surface rather than a field-mapping one, so describe it as the second shape the same gRPC-vs- JSON-RPC/REST split produces instead of implying it dropped a field. Signed-off-by: andy --- .agents/skills/task-state-invariants/SKILL.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.agents/skills/task-state-invariants/SKILL.md b/.agents/skills/task-state-invariants/SKILL.md index 9a9a65ecb..1c5a7ea11 100644 --- a/.agents/skills/task-state-invariants/SKILL.md +++ b/.agents/skills/task-state-invariants/SKILL.md @@ -76,8 +76,10 @@ Each claim is anchored to current `main`. Re-read the file before quoting. 7. **v0.3 compat has two independent mappers.** `compat/v0_3/proto_utils.py` serves the gRPC surface and `compat/v0_3/conversions.py` serves JSON-RPC and REST. A field mapped in one and not the other appears and disappears - by transport — see #1276 (status timestamp dropped on gRPC only) and - #1403. Any compat field change must touch and test both. + by transport — see #1276 (status timestamp mapped for JSON-RPC/REST but + dropped on gRPC). The same split bites error handling rather than fields: + early failures on v0.3 REST streams had to be fixed separately in #1403. + Any compat change must touch and test both mappers. 8. **Continuation identity is inferred, not invented.** With only `message.task_id`, the SDK must reuse the task's `context_id` (A2A v1.0 §3.4.3); generating a fresh one makes events disagree with the