Repository navigation
Conversation
🧪 Code Coverage (vs
|
| Base | PR | Delta | |
|---|---|---|---|
| src/a2a/server/agent_execution/active_task.py | 96.69% | 96.70% | 🟢 +0.01% |
| src/a2a/server/request_handlers/default_request_handler_v2.py | 88.60% | 88.63% | 🟢 +0.03% |
| src/a2a/server/tasks/database_task_store.py | 96.77% | 98.39% | 🟢 +1.61% |
| src/a2a/server/tasks/task_manager.py | 98.95% | 98.99% | 🟢 +0.05% |
| Total | 93.63% | 93.67% | 🟢 +0.04% |
Generated by coverage-comment.yml
c34c53f to
6be10db
Compare
|
Friendly ping — this is ready for review whenever anyone has bandwidth (both branches just rebased onto latest |
mykytanetipa
left a comment
There was a problem hiding this comment.
Why only stamp terminal states in TaskManager?
-
If an agent sends an unstamped WORKING or INPUT_REQUIRED update (for example via new_text_status_update_event, which doesn't set a timestamp), task.status.CopyFrom(event.status) still wipes out the existing timestamp and writes last_updated = NULL to the DB.
-
New SUBMITTED tasks created via new_task_from_user_message or TaskManager._init_task_obj (which ensure_task_id saves directly through _save_task) also start without a timestamp, so tasks created with return_immediately=True or artifact-first flows end up with last_updated = NULL too.
I think we should stamp any status that lacks a timestamp (in save_task_event and _init_task_obj / _save_task), not just terminal ones
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: a2aproject#1276 (status.timestamp mapped for JSON-RPC/REST compat but not gRPC), a2aproject#1188 (handler resumes from a stale snapshot), a2aproject#1206/a2aproject#1411 (contextId inference disagrees between events and the stored task), and the review findings on a2aproject#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 <[email protected]>
6be10db to
c923c37
Compare
|
All review threads here are addressed and resolved (rebased + fixes pushed as c923c37). Thanks for the detailed review — the Summary of what changed after your review:
Local: |
c923c37 to
3b4f247
Compare
Framework failure paths could persist a FAILED task without status.timestamp: the consumer failure path built a TaskStatusUpdateEvent with an unstamped TaskStatus (which TaskManager.save_task_event then CopyFrom'd over the task, clearing any prior timestamp), and the producer failure path mutated task.status.state directly before saving. DatabaseTaskStore derives last_updated from status.timestamp, so the task was persisted with last_updated = NULL, breaking ListTasks ordering and time-based filters (issue a2aproject#1331). Per the approach endorsed on the issue, this lands both layers: - failure sites in ActiveTask (_write_failed_status, _mark_task_as_failed, and the producer failure path) stamp the current time explicitly; - TaskManager.save_task_event gains a defensive backstop that auto-populates the timestamp whenever a terminal state arrives unstamped, covering raw Task and status-event paths alike. Non-terminal states are left untouched, and explicitly provided timestamps are never overwritten. Signed-off-by: andy <[email protected]>
3b4f247 to
a5b7d92
Compare
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: a2aproject#1276 (status.timestamp mapped for JSON-RPC/REST compat but not gRPC), a2aproject#1188 (handler resumes from a stale snapshot), a2aproject#1206/a2aproject#1411 (contextId inference disagrees between events and the stored task), and the review findings on a2aproject#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 <[email protected]>
Line anchors for the cancel-path CopyFrom, the inline push send, the get_task wait, and the copy-on-yield caveat had drifted after the 13-commit rebase onto main (which also merged a2aproject#1345/a2aproject#1346 — the very fixes several anchors point at). All eight anchors re-verified against current code; claim 5 notes the caller-side guard merged via a2aproject#1345. Signed-off-by: andy <[email protected]>
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: a2aproject#1276 (status.timestamp mapped for JSON-RPC/REST compat but not gRPC), a2aproject#1188 (handler resumes from a stale snapshot), a2aproject#1206/a2aproject#1411 (contextId inference disagrees between events and the stored task), and the review findings on a2aproject#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 <[email protected]>
Line anchors for the cancel-path CopyFrom, the inline push send, the get_task wait, and the copy-on-yield caveat had drifted after the 13-commit rebase onto main (which also merged a2aproject#1345/a2aproject#1346 — the very fixes several anchors point at). All eight anchors re-verified against current code; claim 5 notes the caller-side guard merged via a2aproject#1345. Signed-off-by: andy <[email protected]>
Summary
Fixes #1331. Framework failure paths could persist a FAILED task without a
status.timestamp, leavingDatabaseTaskStore.last_updatednull and affecting task ordering and timestamp filtering.TaskManager, supply a timestamp for a missing timestamp on a status in a rawTaskor status-update event, including non-terminal states. Keep an agent-provided timestamp unchanged and stamp the event before copying its status to the stored task.TaskManager, including return-immediately and artifact-first paths.This supplies timestamps for statuses passing through these paths; it does not migrate existing database records. Artifact-only updates to an old task with an unstamped status do not backfill that existing status.
Validation
./scripts/lint.sh(passed; three pre-existingtyunused-ignore warnings).uv run pytest(2,620 passed, 217 skipped, 4 xfailed, 1 xpassed before the additional raw-WORKING test).uv run pytest --cov=src --cov-report=term-missing(2,621 passed, 217 skipped, 4 xfailed, 1 xpassed; 94% coverage).status_timestamp_afterfiltering. PostgreSQL and MySQL cases were skipped because their DSNs were not configured.Prepared with AI assistance.