tsk-6lqhaa [OPEN] A2A read receipts: delivered + seen marks per (mes - #224
tsk-6lqhaa [OPEN] A2A read receipts: delivered + seen marks per (mes#224jaylfc wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds SQLite-backed receipt tracking for message delivery and seen timestamps, including status helpers, upsert and monotonic updates, queries, TTL cleanup, connection management, and registration of ChangesReceipt storage
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
PR Summary by QodoAdd SQLite receipt store for A2A delivered/seen marks
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
| import sqlite3 | ||
| import time | ||
| from pathlib import Path | ||
| from typing import Any, Dict, List, Optional |
There was a problem hiding this comment.
SUGGESTION: Unused imports Any and Dict
Any and Dict are imported but never referenced in this file.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if self.delivered_at is None: | ||
| return False | ||
| # Default TTL of 30 days | ||
| ttl = 30 * 24 * 60 * 60 |
There was a problem hiding this comment.
WARNING: is_expired hardcodes a 30-day TTL
The TTL is hardcoded to 30 days here, but prune_expired accepts a configurable ttl_days parameter. If callers pass a non-default TTL to prune_expired, is_expired will still use 30 days, leading to inconsistent expiration behavior.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| placeholders = ",".join("?" for _ in expired_message_ids) | ||
| cursor = conn.execute( | ||
| f"DELETE FROM receipts WHERE message_id IN ({placeholders})", |
There was a problem hiding this comment.
WARNING: prune_expired over-deletes by collapsing to message_id
The method collects expired message_id values and deletes WHERE message_id IN (...). Because the primary key is (message_id, agent_id), this deletes ALL agent receipts for an expired message, not just the expired (message_id, agent_id) pairs. Non-expired agents' receipts for the same message will be incorrectly removed.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| import sqlite3 | ||
| import time | ||
| from pathlib import Path | ||
| from typing import Any, Dict, List, Optional |
There was a problem hiding this comment.
SUGGESTION: Unused imports Any and Dict
Any and Dict are imported but never referenced in this file.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if self.delivered_at is None: | ||
| return False | ||
| # Default TTL of 30 days | ||
| ttl = 30 * 24 * 60 * 60 |
There was a problem hiding this comment.
WARNING: is_expired hardcodes a 30-day TTL
The TTL is hardcoded to 30 days here, but prune_expired accepts a configurable ttl_days parameter. If callers pass a non-default TTL to prune_expired, is_expired will still use 30 days, leading to inconsistent expiration behavior.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| placeholders = ",".join("?" for _ in expired_message_ids) | ||
| cursor = conn.execute( | ||
| f"DELETE FROM receipts WHERE message_id IN ({placeholders})", |
There was a problem hiding this comment.
WARNING: prune_expired over-deletes by collapsing to message_id
The method collects expired message_id values and deletes WHERE message_id IN (...). Because the primary key is (message_id, agent_id), this deletes ALL agent receipts for an expired message, not just the expired (message_id, agent_id) pairs. Non-expired agents' receipts for the same message will be incorrectly removed.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 6 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 76.3K · Output: 13.7K · Cached: 208.5K |
Code Review by Qodo
Context used✅ Compliance rules (platform):
13 rules 1. Overbroad receipt pruning
|
| expired_message_ids = [] | ||
| for row in rows: | ||
| if now - row["delivered_at"] > ttl_seconds: | ||
| expired_message_ids.append(row["message_id"]) | ||
|
|
||
| if not expired_message_ids: | ||
| return 0 | ||
|
|
||
| placeholders = ",".join("?" for _ in expired_message_ids) | ||
| cursor = conn.execute( | ||
| f"DELETE FROM receipts WHERE message_id IN ({placeholders})", | ||
| expired_message_ids, | ||
| ) |
There was a problem hiding this comment.
1. Overbroad receipt pruning 🐞 Bug ≡ Correctness
ReceiptStore.prune_expired() deletes rows by message_id only, so an expired receipt for one (message_id, agent_id) can delete receipts for other agents that are not expired. This breaks the stated per-(message_id, agent_id) semantics and can cause delivered/seen marks to disappear incorrectly.
Agent Prompt
### Issue description
`prune_expired()` currently builds a list of expired `message_id`s and runs `DELETE FROM receipts WHERE message_id IN (...)`. Because the receipts table key is `(message_id, agent_id)`, this can delete *non-expired* receipts for other agents that share the same message_id.
### Issue Context
Receipts are stored per `(message_id, agent_id)` and should be pruned per-row (or at least per composite key), not per message.
### Fix Focus Areas
- taosmd/receipts.py[240-273]
### Suggested fix approach
- Compute expiry using a cutoff timestamp (`cutoff = now - ttl_seconds`) and delete with a single SQL statement that prunes per-row:
- `DELETE FROM receipts WHERE delivered_at IS NOT NULL AND delivered_at < ?`
- If you need more complex logic later, track and delete by `(message_id, agent_id)` pairs (e.g., `WHERE (message_id, agent_id) IN ((?,?),...)`).
- Consider adding an index on `delivered_at` if prune is frequent.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if self._conn is None: | ||
| path = self._data_dir / "receipts.db" | ||
| path.parent.mkdir(parents=True, exist_ok=True) | ||
| self._conn = sqlite3.connect(str(path)) | ||
| self._conn.row_factory = sqlite3.Row |
There was a problem hiding this comment.
2. Sqlite connect bypasses wal 🐞 Bug ☼ Reliability
ReceiptStore opens SQLite via sqlite3.connect directly, bypassing the project’s taosmd._db.connect() helper that enables WAL and sets busy_timeout. Under concurrent access, this increases the likelihood of database is locked errors and loses the repo-standard observability warning when WAL can’t be enabled.
Agent Prompt
### Issue description
`ReceiptStore._get_conn()` uses `sqlite3.connect()` directly. The repo has a shared connection helper (`taosmd._db.connect`) that sets WAL mode and a busy timeout to reduce SQLITE_BUSY errors.
### Issue Context
Other SQLite-backed stores in this repo consistently use `_db.connect()` before running schema/migrations.
### Fix Focus Areas
- taosmd/receipts.py[87-94]
### Suggested fix approach
- Replace `sqlite3.connect(str(path))` with `taosmd._db.connect(str(path))` (or `_db.connect(path)`), then keep setting `row_factory` as you do now.
- Ensure imports align with other stores (e.g., `from taosmd import _db`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| "claims": "claims.db", | ||
| "collections": "collections.db", | ||
| "knowledge_graph": "knowledge-graph.db", | ||
| "receipts": "receipts.db", |
There was a problem hiding this comment.
3. Receipts db not registered 🐞 Bug ⚙ Maintainability
migrations.DB_FILES now includes a receipts entry, but migrations.REGISTRY does not, so status_all()/migrate_all() won’t include receipts and migrate(conn, 'receipts') would raise “unknown database”. This leaves the new DB inconsistent with the migrations framework used by other stores and blocks future schema evolution via migrations.
Agent Prompt
### Issue description
`DB_FILES` lists `receipts`, but there is no matching `REGISTRY` entry, and migration resolution rejects unknown DB names.
### Issue Context
Other persistent stores call `migrations.migrate(conn, '<db_name>')` and that requires the db to exist in `REGISTRY`.
### Fix Focus Areas
- taosmd/migrations.py[358-419]
- taosmd/migrations.py[395-404]
### Suggested fix approach
- Either:
1) Add a baseline `receipts` migration tuple and include it in `REGISTRY` (and update the receipts store to call `migrations.migrate(conn, 'receipts')`), or
2) Remove the `receipts` entry from `DB_FILES` until the DB is actually managed by migrations.
- Keep `DB_FILES` and `REGISTRY` aligned so DB-wide tooling behaves predictably.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| """Receipt store module for A2A bus message delivery receipts. | ||
|
|
||
| Stores delivered and seen marks for A2A messages per (message_id, agent_id). | ||
| Key: (message_id, agent_id) -> delivered_at (nullable), seen_at (nullable) | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import sqlite3 | ||
| import time | ||
| from pathlib import Path | ||
| from typing import Any, Dict, List, Optional | ||
| from dataclasses import dataclass | ||
|
|
||
|
|
||
| @dataclass | ||
| class Receipt: | ||
| """A2A message receipt record.""" | ||
| message_id: int | ||
| agent_id: str |
There was a problem hiding this comment.
4. Duplicate receipts module 🐞 Bug ⚙ Maintainability
The PR adds taosmd/receipts.py and taosmd/receipts_clean.py with duplicated implementations of Receipt/ReceiptStore. This creates an accidental alternate implementation and increases divergence/maintenance risk (future fixes may land in only one file).
Agent Prompt
### Issue description
Two new modules (`receipts.py` and `receipts_clean.py`) contain the same receipt store implementation.
### Issue Context
Having two copies of the same store encourages drift and confusion about the canonical import path.
### Fix Focus Areas
- taosmd/receipts_clean.py[1-284]
- taosmd/receipts.py[1-284]
### Suggested fix approach
- Decide the canonical module name (likely `taosmd/receipts.py`).
- Either delete `receipts_clean.py`, or make it a thin compatibility shim:
- `from .receipts import Receipt, ReceiptStore` and set `__all__` accordingly.
- If `receipts_clean.py` is intended for an internal purpose, document that explicitly in-module and ensure it is not used as a second source of truth.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
taosmd/receipts.py (1)
1-285: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winConsolidate the duplicate receipt module.
taosmd/receipts.pyandtaosmd/receipts_clean.pyare identical byte-for-byte. Keep a single canonicalReceiptStoremodule and either remove the unused copy or wire imports to the retained module.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@taosmd/receipts.py` around lines 1 - 285, Consolidate the duplicate receipt implementations by retaining one canonical module containing Receipt and ReceiptStore, then remove the unused duplicate receipts_clean module or update all imports and references to use the retained module. Ensure no code continues maintaining or importing two separate copies.
🧹 Nitpick comments (3)
taosmd/receipts.py (3)
275-284: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNo index backs
agent_id-only lookups.The table's only index is the composite
PRIMARY KEY (message_id, agent_id), which SQLite can't use efficiently for a query filtering onagent_idalone. As the table grows,get_all_by_agentwill degrade to a full table scan.⚡ Add a secondary index
CREATE TABLE IF NOT EXISTS receipts ( message_id INTEGER NOT NULL, agent_id TEXT NOT NULL, delivered_at REAL, seen_at REAL, PRIMARY KEY (message_id, agent_id) ) +CREATE INDEX IF NOT EXISTS idx_receipts_agent_id ON receipts(agent_id);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@taosmd/receipts.py` around lines 275 - 284, Add a secondary SQLite index on the receipts table’s agent_id column, using the existing schema/initialization path, so get_all_by_agent can efficiently filter by agent_id. Keep the query and returned Receipt behavior unchanged.
240-273: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
prune_expiredfetches all delivered rows into Python instead of filtering in SQL.This loads every row with
delivered_at IS NOT NULLinto memory and loops in Python to find expired ones, then issues a secondDELETE ... IN (...). A singleDELETE ... WHERE delivered_at < ?is both more efficient and avoids the dynamic placeholder string that triggered the Ruff/OpenGrep SQL-injection hints (which are false positives here since only?characters are interpolated and all values are bound parameters).⚡ Simplify to a single parameterized DELETE
def prune_expired(self, ttl_days: int = 30) -> int: """Remove expired receipts. ... """ conn = self._get_conn() - cursor = conn.execute( - "SELECT message_id, agent_id, delivered_at FROM receipts WHERE delivered_at IS NOT NULL" - ) - rows = cursor.fetchall() - - now = time.time() - ttl_seconds = ttl_days * 24 * 60 * 60 - - expired_message_ids = [] - for row in rows: - if now - row["delivered_at"] > ttl_seconds: - expired_message_ids.append(row["message_id"]) - - if not expired_message_ids: - return 0 - - placeholders = ",".join("?" for _ in expired_message_ids) - cursor = conn.execute( - f"DELETE FROM receipts WHERE message_id IN ({placeholders})", - expired_message_ids, - ) - deleted_count = cursor.rowcount + cutoff = time.time() - (ttl_days * 24 * 60 * 60) + cursor = conn.execute( + "DELETE FROM receipts WHERE delivered_at IS NOT NULL AND delivered_at < ?", + (cutoff,), + ) + deleted_count = cursor.rowcount conn.commit() return deleted_count🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@taosmd/receipts.py` around lines 240 - 273, Update Receipts.prune_expired to remove the SELECT/fetchall loop and dynamic IN-clause construction. Compute the expiration cutoff from time.time() and ttl_days, execute one parameterized DELETE filtering delivered_at before that cutoff, commit the transaction, and return the DELETE rowcount.
1-285: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftNo tests accompany this new persistence module.
Per the PR objectives, the card requested tests and none were added; this needs to land before merge given
record_delivered/record_seencontain the concurrency-sensitive logic flagged below.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@taosmd/receipts.py` around lines 1 - 285, Add focused tests for ReceiptStore, covering record_delivered upsert and skip behavior, record_seen for missing records and timestamp updates, receipt retrieval, and prune_expired. Include concurrency-sensitive scenarios for record_delivered and record_seen, verifying that existing seen timestamps are preserved and updates behave correctly under repeated or concurrent operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@taosmd/receipts_clean.py`:
- Around line 65-73: Remove the unused duplicate _RECEIPTS_SCHEMA definition
from the receipts_clean.py schema section, including the corresponding duplicate
block referenced at the later location. Preserve the actively used receipts
schema definition and any surrounding database initialization behavior.
- Around line 79-94: Align the connection initialization in __init__ and
_get_conn with the shared behavior required by receipts.py: use the same
data_dir default and route database creation through the shared connect() helper
instead of calling sqlite3.connect directly, while preserving row_factory setup
and lazy connection reuse.
- Around line 161-199: Fix the TOCTOU race in record_seen by making the
eligibility check and seen_at update one atomic database operation, using an
UPDATE with conditions that only advances the timestamp (including NULL).
Determine success from the affected-row count, commit only when a row was
updated, and preserve the False result when no matching receipt qualifies.
- Around line 117-159: Make record_delivered atomic by removing the separate
existence SELECT and relying on the INSERT ... ON CONFLICT operation to decide
whether to insert or update. Preserve upsert=False semantics by using an
insert-only conflict path that skips existing rows, and return whether the
database changed a row; keep the existing upsert=True update behavior and commit
handling.
In `@taosmd/receipts.py`:
- Around line 65-73: Update _init_schema to execute the existing
_RECEIPTS_SCHEMA constant instead of duplicating the CREATE TABLE SQL inline,
ensuring the single schema definition remains authoritative and removing the
redundant inline statement.
- Around line 161-199: Update record_seen to perform the monotonic check
atomically by moving the seen_at comparison into the UPDATE statement’s WHERE
clause, allowing updates only when the stored value is null or older than the
new timestamp. Use the UPDATE row count to return True only when a row was
changed and False uniformly when the receipt is missing or already newer; remove
the separate SELECT/check flow while preserving timestamp defaulting and commit
behavior.
- Around line 117-159: Update record_delivered to handle upsert=False with a
single atomic INSERT ... ON CONFLICT DO NOTHING statement, removing the separate
existence SELECT for that path. Return True only when a row is inserted and
False when the conflict skips the write; preserve the existing upsert=True
update behavior and transaction commit handling.
- Around line 79-94: Update ReceiptStore.__init__ to default data_dir through
the canonical taosmd._resolve_data_dir() path, preserving explicit overrides.
Replace the direct sqlite3.connect call in _get_conn with the shared SQLite
connection helper used by other database-backed modules so WAL mode and
busy-timeout settings are applied.
---
Outside diff comments:
In `@taosmd/receipts.py`:
- Around line 1-285: Consolidate the duplicate receipt implementations by
retaining one canonical module containing Receipt and ReceiptStore, then remove
the unused duplicate receipts_clean module or update all imports and references
to use the retained module. Ensure no code continues maintaining or importing
two separate copies.
---
Nitpick comments:
In `@taosmd/receipts.py`:
- Around line 275-284: Add a secondary SQLite index on the receipts table’s
agent_id column, using the existing schema/initialization path, so
get_all_by_agent can efficiently filter by agent_id. Keep the query and returned
Receipt behavior unchanged.
- Around line 240-273: Update Receipts.prune_expired to remove the
SELECT/fetchall loop and dynamic IN-clause construction. Compute the expiration
cutoff from time.time() and ttl_days, execute one parameterized DELETE filtering
delivered_at before that cutoff, commit the transaction, and return the DELETE
rowcount.
- Around line 1-285: Add focused tests for ReceiptStore, covering
record_delivered upsert and skip behavior, record_seen for missing records and
timestamp updates, receipt retrieval, and prune_expired. Include
concurrency-sensitive scenarios for record_delivered and record_seen, verifying
that existing seen timestamps are preserved and updates behave correctly under
repeated or concurrent operations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d7b1ab3-e615-4b67-b9fd-87199a05d704
📒 Files selected for processing (3)
taosmd/migrations.pytaosmd/receipts.pytaosmd/receipts_clean.py
| _RECEIPTS_SCHEMA = """ | ||
| CREATE TABLE IF NOT EXISTS receipts ( | ||
| message_id INTEGER NOT NULL, | ||
| agent_id TEXT NOT NULL, | ||
| delivered_at REAL, | ||
| seen_at REAL, | ||
| PRIMARY KEY (message_id, agent_id) | ||
| ) | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Same unused _RECEIPTS_SCHEMA duplication as taosmd/receipts.py Lines 65-73,96-109.
Also applies to: 96-109
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@taosmd/receipts_clean.py` around lines 65 - 73, Remove the unused duplicate
_RECEIPTS_SCHEMA definition from the receipts_clean.py schema section, including
the corresponding duplicate block referenced at the later location. Preserve the
actively used receipts schema definition and any surrounding database
initialization behavior.
| def __init__( | ||
| self, | ||
| data_dir: str | Path = "data", | ||
| ): | ||
| self._data_dir = Path(data_dir) | ||
| self._conn: sqlite3.Connection | None = None | ||
| self._init_schema() | ||
|
|
||
| def _get_conn(self) -> sqlite3.Connection: | ||
| """Get or create the database connection.""" | ||
| if self._conn is None: | ||
| path = self._data_dir / "receipts.db" | ||
| path.parent.mkdir(parents=True, exist_ok=True) | ||
| self._conn = sqlite3.connect(str(path)) | ||
| self._conn.row_factory = sqlite3.Row | ||
| return self._conn |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Same data_dir default / missing shared connect() helper issue as taosmd/receipts.py Lines 79-94.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@taosmd/receipts_clean.py` around lines 79 - 94, Align the connection
initialization in __init__ and _get_conn with the shared behavior required by
receipts.py: use the same data_dir default and route database creation through
the shared connect() helper instead of calling sqlite3.connect directly, while
preserving row_factory setup and lazy connection reuse.
Source: Coding guidelines
| def record_delivered( | ||
| self, | ||
| message_id: int, | ||
| agent_id: str, | ||
| delivered_at: Optional[float] = None, | ||
| upsert: bool = True, | ||
| ) -> bool: | ||
| """Record that a message was delivered to an agent. | ||
|
|
||
| Args: | ||
| message_id: The ID of the message. | ||
| agent_id: The ID of the agent that received the message. | ||
| delivered_at: When the message was delivered (defaults to now). | ||
| upsert: If True, update existing record. If False, skip if exists. | ||
|
|
||
| Returns: | ||
| True if a new row was inserted or updated, False if skip-if-exists. | ||
| """ | ||
| if delivered_at is None: | ||
| delivered_at = time.time() | ||
|
|
||
| conn = self._get_conn() | ||
| cursor = conn.execute( | ||
| "SELECT delivered_at FROM receipts WHERE message_id = ? AND agent_id = ?", | ||
| (message_id, agent_id), | ||
| ) | ||
| exists = cursor.fetchone() is not None | ||
|
|
||
| if exists and not upsert: | ||
| return False | ||
|
|
||
| conn.execute( | ||
| """ | ||
| INSERT INTO receipts (message_id, agent_id, delivered_at, seen_at) | ||
| VALUES (?, ?, ?, NULL) | ||
| ON CONFLICT(message_id, agent_id) DO UPDATE SET | ||
| delivered_at = excluded.delivered_at, | ||
| seen_at = receipts.seen_at | ||
| """, | ||
| (message_id, agent_id, delivered_at), | ||
| ) | ||
| conn.commit() | ||
| return True |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Same TOCTOU race in record_delivered as taosmd/receipts.py Lines 117-159.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@taosmd/receipts_clean.py` around lines 117 - 159, Make record_delivered
atomic by removing the separate existence SELECT and relying on the INSERT ...
ON CONFLICT operation to decide whether to insert or update. Preserve
upsert=False semantics by using an insert-only conflict path that skips existing
rows, and return whether the database changed a row; keep the existing
upsert=True update behavior and commit handling.
| def record_seen( | ||
| self, | ||
| message_id: int, | ||
| agent_id: str, | ||
| seen_at: Optional[float] = None, | ||
| ) -> bool: | ||
| """Record that an agent has seen a message. | ||
|
|
||
| Args: | ||
| message_id: The ID of the message. | ||
| agent_id: The ID of the agent that saw the message. | ||
| seen_at: When the message was seen (defaults to now). | ||
|
|
||
| Returns: | ||
| True if the seen_at was updated, False if the record doesn't exist. | ||
| """ | ||
| if seen_at is None: | ||
| seen_at = time.time() | ||
|
|
||
| conn = self._get_conn() | ||
| cursor = conn.execute( | ||
| "SELECT seen_at FROM receipts WHERE message_id = ? AND agent_id = ?", | ||
| (message_id, agent_id), | ||
| ) | ||
| row = cursor.fetchone() | ||
| if row is None: | ||
| return False | ||
|
|
||
| existing_seen = row["seen_at"] | ||
| # Only update if moving from null to a value | ||
| if existing_seen is not None and existing_seen >= seen_at: | ||
| return False | ||
|
|
||
| conn.execute( | ||
| "UPDATE receipts SET seen_at = ? WHERE message_id = ? AND agent_id = ?", | ||
| (seen_at, message_id, agent_id), | ||
| ) | ||
| conn.commit() | ||
| return True |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Same TOCTOU race in record_seen as taosmd/receipts.py Lines 161-199.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@taosmd/receipts_clean.py` around lines 161 - 199, Fix the TOCTOU race in
record_seen by making the eligibility check and seen_at update one atomic
database operation, using an UPDATE with conditions that only advances the
timestamp (including NULL). Determine success from the affected-row count,
commit only when a row was updated, and preserve the False result when no
matching receipt qualifies.
| _RECEIPTS_SCHEMA = """ | ||
| CREATE TABLE IF NOT EXISTS receipts ( | ||
| message_id INTEGER NOT NULL, | ||
| agent_id TEXT NOT NULL, | ||
| delivered_at REAL, | ||
| seen_at REAL, | ||
| PRIMARY KEY (message_id, agent_id) | ||
| ) | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Unused _RECEIPTS_SCHEMA constant duplicated inline in _init_schema.
The module-level _RECEIPTS_SCHEMA string is never referenced; _init_schema re-writes the identical CREATE TABLE SQL inline instead. Any future schema tweak risks updating only one copy.
♻️ Use the constant instead of duplicating the SQL
def _init_schema(self) -> None:
"""Initialize the database schema."""
conn = self._get_conn()
- conn.execute(
- """
- CREATE TABLE IF NOT EXISTS receipts (
- message_id INTEGER NOT NULL,
- agent_id TEXT NOT NULL,
- delivered_at REAL,
- seen_at REAL,
- PRIMARY KEY (message_id, agent_id)
- )
- """
- )
+ conn.execute(_RECEIPTS_SCHEMA)Also applies to: 96-109
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@taosmd/receipts.py` around lines 65 - 73, Update _init_schema to execute the
existing _RECEIPTS_SCHEMA constant instead of duplicating the CREATE TABLE SQL
inline, ensuring the single schema definition remains authoritative and removing
the redundant inline statement.
| def __init__( | ||
| self, | ||
| data_dir: str | Path = "data", | ||
| ): | ||
| self._data_dir = Path(data_dir) | ||
| self._conn: sqlite3.Connection | None = None | ||
| self._init_schema() | ||
|
|
||
| def _get_conn(self) -> sqlite3.Connection: | ||
| """Get or create the database connection.""" | ||
| if self._conn is None: | ||
| path = self._data_dir / "receipts.db" | ||
| path.parent.mkdir(parents=True, exist_ok=True) | ||
| self._conn = sqlite3.connect(str(path)) | ||
| self._conn.row_factory = sqlite3.Row | ||
| return self._conn |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: check how sibling stores (archive/knowledge_graph/etc.) default data_dir.
rg -n --type=py -C2 'data_dir\s*:\s*.*=\s*' taosmd/archive.py taosmd/knowledge_graph.py taosmd/session_catalog.py taosmd/vector_memory.py taosmd/collections.py taosmd/claims/store.pyRepository: jaylfc/taosmd
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tracked receipt and DB files:"
git ls-files | rg '(^|/)receipts\.py$|(^|/)_db\.py$|(^|/)(archive|knowledge_graph|session_catalog|vector_memory|collections).*\.py$' || true
echo
echo "Receipts outline:"
ast-grep outline taosmd/receipts.py --view expanded || true
echo
echo "Relevant receipts lines:"
cat -n taosmd/receipts.py | sed -n '1,140p'
echo
echo "_db outline and connect helper content:"
cat -n taosmd/_db.py | sed -n '1,220p'
echo
echo "Search data_dir defaults across tracked py files:"
rg -n --type=py -C1 'data_dir\s*:\s*.*=\s*|Home|home\(\)|__init__' taosmd || true
echo
echo "Search sqlite3 direct connects and connect helper usages:"
rg -n --type=py -C1 'sqlite3\.connect|from taosmd\._db import|connect\(' taosmd || trueRepository: jaylfc/taosmd
Length of output: 25458
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate DEFAULT_DATA_DIR and taOSmd data dir resolver:"
rg -n --type=py -C2 'DEFAULT_DATA_DIR|TAOSMD_DATA_DIR|Path\.home\(\) \* "\.taosmd"|\.taosmd|data_dir\s*:\s*(Path|str|None)=\s*' taosmd || true
echo
echo "Auto setup data handling:"
cat -n taosmd/auto_setup.py | sed -n '1,120p'
echo
echo "Agent data_dir resolver and store construction:"
cat -n taosmd/agents.py | sed -n '140,210p'
echo
echo "Existing stores using shared _db.connect:"
for f in taosmd/archive.py taosmd/collections.py taosmd/taosmd_backend.py; do
echo "--- $f"
cat -n "$f" | sed -n '1,120p'
doneRepository: jaylfc/taosmd
Length of output: 40384
Align ReceiptStore with the canonical ~/.taosmd/ data path and reuse the shared SQLite helper.
data_dir: str | Path = "data" creates the receipt database under CWD’s data/, diverging from taosmd._resolve_data_dir() / ~/.taosmd/. Also, sqlite3.connect leaves this store out of WAL mode and busy-timeout protection that other DB-backed modules use for concurrent access.
🔧 Proposed fix
+from taosmd._db import connect as _db_connect
+
class ReceiptStore:
"""Store for A2A message delivery receipts."""
def __init__(
self,
- data_dir: str | Path = "data",
+ data_dir: str | Path = Path.home() / ".taosmd",
):
self._data_dir = Path(data_dir)
self._conn: sqlite3.Connection | None = None
self._init_schema()
def _get_conn(self) -> sqlite3.Connection:
"""Get or create the database connection."""
if self._conn is None:
path = self._data_dir / "receipts.db"
path.parent.mkdir(parents=True, exist_ok=True)
- self._conn = sqlite3.connect(str(path))
+ self._conn = _db_connect(path)
self._conn.row_factory = sqlite3.Row
self._conn.row_factory = sqlite3.Row
return self._conn📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def __init__( | |
| self, | |
| data_dir: str | Path = "data", | |
| ): | |
| self._data_dir = Path(data_dir) | |
| self._conn: sqlite3.Connection | None = None | |
| self._init_schema() | |
| def _get_conn(self) -> sqlite3.Connection: | |
| """Get or create the database connection.""" | |
| if self._conn is None: | |
| path = self._data_dir / "receipts.db" | |
| path.parent.mkdir(parents=True, exist_ok=True) | |
| self._conn = sqlite3.connect(str(path)) | |
| self._conn.row_factory = sqlite3.Row | |
| return self._conn | |
| from taosmd._db import connect as _db_connect | |
| class ReceiptStore: | |
| """Store for A2A message delivery receipts.""" | |
| def __init__( | |
| self, | |
| data_dir: str | Path = Path.home() / ".taosmd", | |
| ): | |
| self._data_dir = Path(data_dir) | |
| self._conn: sqlite3.Connection | None = None | |
| self._init_schema() | |
| def _get_conn(self) -> sqlite3.Connection: | |
| """Get or create the database connection.""" | |
| if self._conn is None: | |
| path = self._data_dir / "receipts.db" | |
| path.parent.mkdir(parents=True, exist_ok=True) | |
| self._conn = _db_connect(path) | |
| self._conn.row_factory = sqlite3.Row | |
| self._conn.row_factory = sqlite3.Row | |
| return self._conn |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@taosmd/receipts.py` around lines 79 - 94, Update ReceiptStore.__init__ to
default data_dir through the canonical taosmd._resolve_data_dir() path,
preserving explicit overrides. Replace the direct sqlite3.connect call in
_get_conn with the shared SQLite connection helper used by other database-backed
modules so WAL mode and busy-timeout settings are applied.
Source: Coding guidelines
| def record_delivered( | ||
| self, | ||
| message_id: int, | ||
| agent_id: str, | ||
| delivered_at: Optional[float] = None, | ||
| upsert: bool = True, | ||
| ) -> bool: | ||
| """Record that a message was delivered to an agent. | ||
|
|
||
| Args: | ||
| message_id: The ID of the message. | ||
| agent_id: The ID of the agent that received the message. | ||
| delivered_at: When the message was delivered (defaults to now). | ||
| upsert: If True, update existing record. If False, skip if exists. | ||
|
|
||
| Returns: | ||
| True if a new row was inserted or updated, False if skip-if-exists. | ||
| """ | ||
| if delivered_at is None: | ||
| delivered_at = time.time() | ||
|
|
||
| conn = self._get_conn() | ||
| cursor = conn.execute( | ||
| "SELECT delivered_at FROM receipts WHERE message_id = ? AND agent_id = ?", | ||
| (message_id, agent_id), | ||
| ) | ||
| exists = cursor.fetchone() is not None | ||
|
|
||
| if exists and not upsert: | ||
| return False | ||
|
|
||
| conn.execute( | ||
| """ | ||
| INSERT INTO receipts (message_id, agent_id, delivered_at, seen_at) | ||
| VALUES (?, ?, ?, NULL) | ||
| ON CONFLICT(message_id, agent_id) DO UPDATE SET | ||
| delivered_at = excluded.delivered_at, | ||
| seen_at = receipts.seen_at | ||
| """, | ||
| (message_id, agent_id, delivered_at), | ||
| ) | ||
| conn.commit() | ||
| return True |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
TOCTOU race between the existence check and the upsert in record_delivered.
When upsert=False, the method does a separate SELECT to decide whether to skip, then a separate INSERT ... ON CONFLICT DO UPDATE. If this store/DB file is accessed from more than one connection concurrently (multiple processes/threads), two callers can both observe "not exists" and both proceed, so the second write silently overwrites delivered_at even though upsert=False was requested. SQLite's ON CONFLICT ... DO NOTHING makes the skip-if-exists path atomic and removes the race entirely.
🔒 Atomic skip-if-exists via DO NOTHING
- conn = self._get_conn()
- cursor = conn.execute(
- "SELECT delivered_at FROM receipts WHERE message_id = ? AND agent_id = ?",
- (message_id, agent_id),
- )
- exists = cursor.fetchone() is not None
-
- if exists and not upsert:
- return False
-
- conn.execute(
- """
- INSERT INTO receipts (message_id, agent_id, delivered_at, seen_at)
- VALUES (?, ?, ?, NULL)
- ON CONFLICT(message_id, agent_id) DO UPDATE SET
- delivered_at = excluded.delivered_at,
- seen_at = receipts.seen_at
- """,
- (message_id, agent_id, delivered_at),
- )
- conn.commit()
- return True
+ conn = self._get_conn()
+ conflict_action = (
+ "DO UPDATE SET delivered_at = excluded.delivered_at, "
+ "seen_at = receipts.seen_at"
+ if upsert
+ else "DO NOTHING"
+ )
+ cursor = conn.execute(
+ f"""
+ INSERT INTO receipts (message_id, agent_id, delivered_at, seen_at)
+ VALUES (?, ?, ?, NULL)
+ ON CONFLICT(message_id, agent_id) {conflict_action}
+ """,
+ (message_id, agent_id, delivered_at),
+ )
+ conn.commit()
+ return upsert or cursor.rowcount > 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def record_delivered( | |
| self, | |
| message_id: int, | |
| agent_id: str, | |
| delivered_at: Optional[float] = None, | |
| upsert: bool = True, | |
| ) -> bool: | |
| """Record that a message was delivered to an agent. | |
| Args: | |
| message_id: The ID of the message. | |
| agent_id: The ID of the agent that received the message. | |
| delivered_at: When the message was delivered (defaults to now). | |
| upsert: If True, update existing record. If False, skip if exists. | |
| Returns: | |
| True if a new row was inserted or updated, False if skip-if-exists. | |
| """ | |
| if delivered_at is None: | |
| delivered_at = time.time() | |
| conn = self._get_conn() | |
| cursor = conn.execute( | |
| "SELECT delivered_at FROM receipts WHERE message_id = ? AND agent_id = ?", | |
| (message_id, agent_id), | |
| ) | |
| exists = cursor.fetchone() is not None | |
| if exists and not upsert: | |
| return False | |
| conn.execute( | |
| """ | |
| INSERT INTO receipts (message_id, agent_id, delivered_at, seen_at) | |
| VALUES (?, ?, ?, NULL) | |
| ON CONFLICT(message_id, agent_id) DO UPDATE SET | |
| delivered_at = excluded.delivered_at, | |
| seen_at = receipts.seen_at | |
| """, | |
| (message_id, agent_id, delivered_at), | |
| ) | |
| conn.commit() | |
| return True | |
| def record_delivered( | |
| self, | |
| message_id: int, | |
| agent_id: str, | |
| delivered_at: Optional[float] = None, | |
| upsert: bool = True, | |
| ) -> bool: | |
| """Record that a message was delivered to an agent. | |
| Args: | |
| message_id: The ID of the message. | |
| agent_id: The ID of the agent that received the message. | |
| delivered_at: When the message was delivered (defaults to now). | |
| upsert: If True, update existing record. If False, skip if exists. | |
| Returns: | |
| True if a new row was inserted or updated, False if skip-if-exists. | |
| """ | |
| if delivered_at is None: | |
| delivered_at = time.time() | |
| conn = self._get_conn() | |
| conflict_action = ( | |
| "DO UPDATE SET delivered_at = excluded.delivered_at, " | |
| "seen_at = receipts.seen_at" | |
| if upsert | |
| else "DO NOTHING" | |
| ) | |
| cursor = conn.execute( | |
| f""" | |
| INSERT INTO receipts (message_id, agent_id, delivered_at, seen_at) | |
| VALUES (?, ?, ?, NULL) | |
| ON CONFLICT(message_id, agent_id) {conflict_action} | |
| """, | |
| (message_id, agent_id, delivered_at), | |
| ) | |
| conn.commit() | |
| return upsert or cursor.rowcount > 0 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@taosmd/receipts.py` around lines 117 - 159, Update record_delivered to handle
upsert=False with a single atomic INSERT ... ON CONFLICT DO NOTHING statement,
removing the separate existence SELECT for that path. Return True only when a
row is inserted and False when the conflict skips the write; preserve the
existing upsert=True update behavior and transaction commit handling.
| def record_seen( | ||
| self, | ||
| message_id: int, | ||
| agent_id: str, | ||
| seen_at: Optional[float] = None, | ||
| ) -> bool: | ||
| """Record that an agent has seen a message. | ||
|
|
||
| Args: | ||
| message_id: The ID of the message. | ||
| agent_id: The ID of the agent that saw the message. | ||
| seen_at: When the message was seen (defaults to now). | ||
|
|
||
| Returns: | ||
| True if the seen_at was updated, False if the record doesn't exist. | ||
| """ | ||
| if seen_at is None: | ||
| seen_at = time.time() | ||
|
|
||
| conn = self._get_conn() | ||
| cursor = conn.execute( | ||
| "SELECT seen_at FROM receipts WHERE message_id = ? AND agent_id = ?", | ||
| (message_id, agent_id), | ||
| ) | ||
| row = cursor.fetchone() | ||
| if row is None: | ||
| return False | ||
|
|
||
| existing_seen = row["seen_at"] | ||
| # Only update if moving from null to a value | ||
| if existing_seen is not None and existing_seen >= seen_at: | ||
| return False | ||
|
|
||
| conn.execute( | ||
| "UPDATE receipts SET seen_at = ? WHERE message_id = ? AND agent_id = ?", | ||
| (seen_at, message_id, agent_id), | ||
| ) | ||
| conn.commit() | ||
| return True |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
TOCTOU race in record_seen's monotonic check.
The read (SELECT seen_at) and the conditional UPDATE are two separate statements. Under concurrent writers, both can pass the monotonic check with a stale read and then apply their UPDATE in the wrong order, letting an older seen_at clobber a newer one. Fold the monotonic condition into the UPDATE's WHERE clause so the check-and-set is atomic.
🔒 Atomic monotonic update
- conn = self._get_conn()
- cursor = conn.execute(
- "SELECT seen_at FROM receipts WHERE message_id = ? AND agent_id = ?",
- (message_id, agent_id),
- )
- row = cursor.fetchone()
- if row is None:
- return False
-
- existing_seen = row["seen_at"]
- # Only update if moving from null to a value
- if existing_seen is not None and existing_seen >= seen_at:
- return False
-
- conn.execute(
- "UPDATE receipts SET seen_at = ? WHERE message_id = ? AND agent_id = ?",
- (seen_at, message_id, agent_id),
- )
- conn.commit()
- return True
+ conn = self._get_conn()
+ cursor = conn.execute(
+ """
+ UPDATE receipts SET seen_at = ?
+ WHERE message_id = ? AND agent_id = ?
+ AND (seen_at IS NULL OR seen_at < ?)
+ """,
+ (seen_at, message_id, agent_id, seen_at),
+ )
+ conn.commit()
+ return cursor.rowcount > 0Note: this also changes the "record doesn't exist" and "already newer" cases to both return False uniformly, matching the original ambiguity but without the race.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def record_seen( | |
| self, | |
| message_id: int, | |
| agent_id: str, | |
| seen_at: Optional[float] = None, | |
| ) -> bool: | |
| """Record that an agent has seen a message. | |
| Args: | |
| message_id: The ID of the message. | |
| agent_id: The ID of the agent that saw the message. | |
| seen_at: When the message was seen (defaults to now). | |
| Returns: | |
| True if the seen_at was updated, False if the record doesn't exist. | |
| """ | |
| if seen_at is None: | |
| seen_at = time.time() | |
| conn = self._get_conn() | |
| cursor = conn.execute( | |
| "SELECT seen_at FROM receipts WHERE message_id = ? AND agent_id = ?", | |
| (message_id, agent_id), | |
| ) | |
| row = cursor.fetchone() | |
| if row is None: | |
| return False | |
| existing_seen = row["seen_at"] | |
| # Only update if moving from null to a value | |
| if existing_seen is not None and existing_seen >= seen_at: | |
| return False | |
| conn.execute( | |
| "UPDATE receipts SET seen_at = ? WHERE message_id = ? AND agent_id = ?", | |
| (seen_at, message_id, agent_id), | |
| ) | |
| conn.commit() | |
| return True | |
| def record_seen( | |
| self, | |
| message_id: int, | |
| agent_id: str, | |
| seen_at: Optional[float] = None, | |
| ) -> bool: | |
| """Record that an agent has seen a message. | |
| Args: | |
| message_id: The ID of the message. | |
| agent_id: The ID of the agent that saw the message. | |
| seen_at: When the message was seen (defaults to now). | |
| Returns: | |
| True if the seen_at was updated, False if the record doesn't exist. | |
| """ | |
| if seen_at is None: | |
| seen_at = time.time() | |
| conn = self._get_conn() | |
| cursor = conn.execute( | |
| """ | |
| UPDATE receipts SET seen_at = ? | |
| WHERE message_id = ? AND agent_id = ? | |
| AND (seen_at IS NULL OR seen_at < ?) | |
| """, | |
| (seen_at, message_id, agent_id, seen_at), | |
| ) | |
| conn.commit() | |
| return cursor.rowcount > 0 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@taosmd/receipts.py` around lines 161 - 199, Update record_seen to perform the
monotonic check atomically by moving the seen_at comparison into the UPDATE
statement’s WHERE clause, allowing updates only when the stored value is null or
older than the new timestamp. Use the UPDATE row count to return True only when
a row was changed and False uniformly when the receipt is missing or already
newer; remove the separate SELECT/check flow while preserving timestamp
defaulting and commit behavior.
|
Closing for rework. The receipts store is committed but nothing imports it: no endpoint, no service function, no MCP tool, no delivery hook, so the feature is 0 percent delivered. receipts_clean.py is a byte-identical stray duplicate. The DB_FILES entry bypasses the real REGISTRY migration framework and is used by nothing. prune_expired deletes by message_id only, wiping every agent's receipts for that message. Zero tests. Redo: wire delivered/seen marking into the actual bus paths, migrate via REGISTRY, key deletes on (message_id, agent_id), tests in tests/, and state clearly how receipt identity is bound given the bus accepts self-claimed senders. |
Autonomous build of board card tsk-6lqhaa.
Files:
taosmd/migrations.py | 1 +
taosmd/receipts.py | 284 +++++++++++++++++++++++++++++++++++++++++++++++
taosmd/receipts_clean.py | 284 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 569 insertions(+)
Summary by Gitar
Receiptdataclass andReceiptStoreintaosmd/receipts.pyandtaosmd/receipts_clean.pyreceiptsdatabase configuration toDB_FILESmapping intaosmd/migrations.pyThis will update automatically on new commits.
Summary by CodeRabbit