tsk-6lqhaa [OPEN] A2A read receipts: delivered + seen marks per (mes - #232
tsk-6lqhaa [OPEN] A2A read receipts: delivered + seen marks per (mes#232jaylfc wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Good progress and a real improvement on the rejected PR 224, which was entirely unwired dead code. This one compiles, is wired into service.py, http_server.py and api.py, and registers the migration with a proper baseline function rather than smuggling a table in via a schema constant. That was the main thing I asked for. It cannot merge yet: the diff contains ZERO test files. Receipts are now on the critical path (priority 90) because the entry badges in the taOS collaboration design depend on an honest 'seen' state, and because without receipts an agent cannot know it has already handled a message and re-reads at full token cost. So this needs to be right rather than quick. Tests required before I review the logic, so we only do that once:
RED-FIRST on 1 and 4. One thing to state in the PR body rather than leave implicit: on a bus where the sender is self-claimed, a receipt is ADVISORY. Any client can mark any message as seen by any agent. That is acceptable for a badge, and unacceptable if anything ever gates on it. Say so in the docstring so the next reader does not assume more than it delivers. This is why the identity work (tsk-legqtr) sits above receipts in the ordering. Also retitle: the title is the card id and truncated text. Sixth taosmd PR in a row with a title that does not describe the change; there is now a card for fixing that mechanically (tsk-nwyrtg). |
PR Summary by QodoAdd A2A delivered/seen receipts with HTTP endpoints and SQLite backing store
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
13 rules 1. Missing time import
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| stores = await _api._ensure_stores(data_dir) | ||
| receipt_store = stores["receipts"] | ||
| await receipt_store.record_delivered(message_id, agent_id, ts) | ||
| return {"ok": True} |
There was a problem hiding this comment.
1. Receipts endpoints not archived 📘 Rule violation ☼ Reliability
The new A2A receipt write/read endpoints update state and return results without any archive.record(...) call, so these interactions can occur without being centrally archived. This reduces auditability and violates the requirement to archive every interaction regardless of success/failure paths.
Agent Prompt
## Issue description
New receipt-related HTTP entry points (and their service methods) do not call `archive.record(...)`, allowing interactions (delivered/seen marks and receipt reads) to happen without being archived.
## Issue Context
Compliance requires each public entry point that processes an interaction to invoke the centralized archiving utility exactly once per processed interaction, including error paths.
## Fix Focus Areas
- taosmd/http_server.py[1656-1700]
- taosmd/http_server.py[1633-1644]
- taosmd/service.py[628-723]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if ts is None: | ||
| ts = time.time() | ||
| remote = _get_remote(data_dir) | ||
| if remote is not None: | ||
| return await remote.a2a_record_delivered(message_id, agent_id, ts=ts) |
There was a problem hiding this comment.
2. Missing time import 🐞 Bug ≡ Correctness
taosmd.service.a2a_record_seen/a2a_record_delivered call time.time() but taosmd/service.py never imports time, causing NameError when ts is omitted (including the new PATCH /a2a/receipts path). This makes seen receipt marking fail at runtime.
Agent Prompt
## Issue description
`taosmd/service.py` uses `time.time()` in the new A2A receipt helpers, but the module does not import `time`. Any call that relies on the default `ts` (not passed explicitly) raises `NameError` and breaks the new seen-receipt endpoint.
## Issue Context
The HTTP handler `_handle_a2a_receipts_seen()` calls `service.a2a_record_seen(..., data_dir=...)` without a `ts`, so this is immediately user-facing.
## Fix Focus Areas
- taosmd/service.py[26-35]
- taosmd/service.py[628-671]
- taosmd/http_server.py[1656-1672]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| remote = _get_remote(data_dir) | ||
| if remote is not None: | ||
| return await remote.a2a_record_seen(message_id, agent_id, ts=ts) | ||
| stores = await _api._ensure_stores(data_dir) |
There was a problem hiding this comment.
3. Remote receipts methods missing 🐞 Bug ≡ Correctness
When a remote server URL is configured, the new receipt service functions forward to remote.a2a_* methods that do not exist on RemoteClient, causing AttributeError and disabling receipts in remote mode. This impacts delivered/seen recording and receipt reads/pruning whenever _get_remote() returns a client.
Agent Prompt
## Issue description
`taosmd.service` now forwards receipt operations to `RemoteClient` when a remote server is configured, but `RemoteClient` does not implement `a2a_record_delivered`, `a2a_record_seen`, `a2a_get_receipts`, `a2a_get_receipt`, or `a2a_prune_receipts`. This will raise `AttributeError` at runtime in remote mode.
## Issue Context
Remote dispatch is a core feature of `taosmd.service` (transparent remote execution). The new receipt methods must either:
1) be implemented end-to-end (HTTP endpoints + RemoteClient methods), or
2) be explicitly blocked in remote mode with a controlled error.
## Fix Focus Areas
- taosmd/service.py[628-723]
- taosmd/remote.py[206-260]
- taosmd/http_server.py[1654-1700]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| import jwt as _jwt # noqa: PLC0415 | ||
| unverified = _jwt.decode(token, options={"verify_signature": False}) | ||
| sub = unverified.get("sub", "") or "" | ||
| return sub if sub else None |
There was a problem hiding this comment.
4. Jwt sub not verified 🐞 Bug ⛨ Security
_get_authenticated_agent_id() decodes the Bearer token with signature verification disabled and trusts the unverified sub claim to attribute delivered/seen receipts, so a forged token can spoof another agent’s identity in receipts. This bypasses the registry verifier’s signature/revocation checks and undermines receipt integrity.
Agent Prompt
## Issue description
`taosmd/http_server.py::_get_authenticated_agent_id` extracts `sub` from a JWT decoded with `verify_signature=False` and returns it as the authenticated identity. This value is then used to write receipt rows, enabling identity spoofing.
## Issue Context
A registry verifier already exists (`RegistryVerifier.authorize`) and performs EdDSA signature verification + revocation checks. Receipt attribution should only use `sub` from verified claims.
## Fix Focus Areas
- taosmd/http_server.py[729-752]
- taosmd/http_server.py[1609-1644]
- taosmd/http_server.py[1656-1672]
- taosmd/registry_auth.py[48-80]
- taosmd/registry_auth.py[139-145]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| elif method == "POST" and path == "/a2a/admin/prune-receipts": | ||
| self._handle_admin_a2a_prune_receipts() |
There was a problem hiding this comment.
5. Prune handler missing 🐞 Bug ≡ Correctness
POST /a2a/admin/prune-receipts is routed to _handle_admin_a2a_prune_receipts() but that handler is not defined on the HTTP handler class, causing an AttributeError (500) when the endpoint is called. The route is therefore unusable as implemented.
Agent Prompt
## Issue description
The router dispatches `POST /a2a/admin/prune-receipts` to `_handle_admin_a2a_prune_receipts()`, but the handler method is missing.
## Issue Context
Other admin A2A handlers exist in the same section (`_handle_admin_a2a_delete_channel`, `_handle_admin_a2a_rename_channel`, `_handle_admin_a2a_supersede_message`). Prune should follow the same pattern: enforce `_check_admin_token()`, parse `ttl_days`, compute cutoff timestamp, call `service.a2a_prune_receipts(...)`, return `{"pruned": int}`.
## Fix Focus Areas
- taosmd/http_server.py[1083-1090]
- taosmd/http_server.py[2119-2169]
- taosmd/service.py[706-723]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| async def init(self) -> None: | ||
| self._conn = sqlite3.connect(self._db_path, check_same_thread=False) | ||
| self._conn.row_factory = sqlite3.Row | ||
| self._conn.executescript(SCHEMA) | ||
| self._conn.commit() |
There was a problem hiding this comment.
6. Receiptstore skips migrations/wal 🐞 Bug ☼ Reliability
ReceiptStore.init() uses sqlite3.connect directly and does not run `migrations.migrate(..., "a2a_receipts")`, unlike other stores, so WAL/busy_timeout pragmas and schema versioning are not applied to the receipts DB. This increases the risk of database is locked under contention and makes future upgrades/migrations for a2a-receipts.db unreliable.
Agent Prompt
## Issue description
`ReceiptStore` initializes SQLite differently from the rest of the codebase: it bypasses `taosmd._db.connect` (WAL + busy timeout) and does not invoke `migrations.migrate` even though a migration registry entry for `a2a_receipts` was added.
## Issue Context
Other stores (e.g., `ClaimStore`) follow the standard pattern: `_db.connect(...)`, `executescript(SCHEMA)`, then `migrations.migrate(conn, "<logical_db_name>")`. The migrations registry now includes `a2a_receipts`, suggesting receipts should participate in the same versioning/upgrade system.
## Fix Focus Areas
- taosmd/receipts.py[56-61]
- taosmd/_db.py[29-59]
- taosmd/claims/store.py[40-47]
- taosmd/migrations.py[305-318]
- taosmd/migrations.py[374-421]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
CHANGES REQUESTED — a dispatched handler does not exist, and there are no testsSubstantive review. Two blockers, one of which is the exact defect class this PR was reworked to fix. Blocker 1 —
|
|
Requesting changes. The store design is right (idempotent delivered mark, monotonic seen, prune by delivered_at) and the schema and service wrappers are clean. But four things are broken, and I found them by running the branch rather than reading it, so each one below has its evidence attached along with the positive control that makes the evidence mean something. 1. BLOCKER: the receipt identity is not verified, so any client can write receipts as any agent
This is not a theoretical gap. The correct API is already in the same class, 100 lines above: Proof, from the PR head. A token signed with a key the server has never seen: The last line is there on purpose: the control accepts a legitimately signed token, so its rejection of the forgery is a real rejection and not a broken harness. The negative control shows the function is not simply returning everything it is handed. Consequence: Fix: call 2. BLOCKER:
|
|
Revision card filed: This PR is not being abandoned and it stays open. The card's STEP 0 squash-merges this branch so the work carries forward. The reason it needs a new card rather than a revision of this one's card is mechanical, and I checked it in source today rather than assuming the tooling had landed: STEP 0 uses Findings are unchanged from my review above; the card summarises them and points back here as the authoritative detail. |
|
Closing as superseded by #270. The review of this PR asked for the revision; Verified before closing: #270 branches off This PR was holding one of the 8 CI throttle slots on |
1 participant
Autonomous build of board card tsk-6lqhaa.
Files:
taosmd/api.py | 4 ++
taosmd/http_server.py | 110 ++++++++++++++++++++++++++++++++++-
taosmd/migrations.py | 18 ++++++
taosmd/receipts.py | 158 ++++++++++++++++++++++++++++++++++++++++++++++++++
taosmd/service.py | 98 +++++++++++++++++++++++++++++++
5 files changed, 387 insertions(+), 1 deletion(-)
Summary by Gitar
ReceiptStoreintaosmd/receipts.pyfor tracking A2A delivery and seen receipts with idempotent insertion and monotonic updatestaosmd/http_server.pyfor recording seen receipts, querying message receipts, and administrative pruninga2a-receipts.dbstoreThis will update automatically on new commits.