chore: drop generated artifacts not tracked on master - #233
Conversation
📝 WalkthroughWalkthroughAdds recipient-aware A2A sending, SQLite-backed mention indexing, filtered mention feeds with thread visibility checks, and authenticated HTTP and remote client access. ChangesA2A mention feeds
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RemoteClient
participant HTTPServer
participant Service
participant MentionStore
participant Archive
RemoteClient->>HTTPServer: GET /a2a/mentions
HTTPServer->>HTTPServer: Validate token and pagination
HTTPServer->>Service: Call a2a_mentions_feed with reader
Service->>MentionStore: Query mention records
Service->>Archive: Resolve reply chains and thread roots
Archive-->>Service: Return message records
Service-->>HTTPServer: Return filtered messages
HTTPServer-->>RemoteClient: Return messages list
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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 A2A mention indexing and authenticated /a2a/mentions feed
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (5)
taosmd/mentions.py (2)
25-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign
initwith the other store classes.Two differences from the peer stores:
ClaimStore.init,TemporalKnowledgeGraph.init, andArchiveStore.initcreate the parent directory before connecting.MentionStore.initdoes not. Today_ensure_storescreates the data dir first, so this works. Any other caller that constructsMentionStoredirectly gets a connect failure.- The peer stores call
migrations.migrate(self._conn, "<name>")afterexecutescript.MentionStorehas no migration namespace, so a later change to thementionsschema has no upgrade path for existinga2a-mentions.dbfiles.Item 1 is a one-line fix. Item 2 is worth deciding now, before the table ships.
♻️ Proposed fix for item 1
+from pathlib import Path + async def init(self) -> None: + Path(self._db_path).parent.mkdir(parents=True, exist_ok=True) self._conn = _db.connect(self._db_path)🤖 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/mentions.py` around lines 25 - 38, Update MentionStore.init to create the parent directory before calling _db.connect, matching ClaimStore.init, TemporalKnowledgeGraph.init, and ArchiveStore.init. Also add a migrations.migrate call after the mentions executescript using a dedicated, stable migration namespace for this store.
59-65: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a uniqueness constraint to match the documented key.
The class docstring declares the index is keyed by
(mentioned_handle, message_id, ts, thread), but the schema has no UNIQUE constraint. A secondrecord_mentionscall for the samemessage_idinserts duplicate rows.get_mentioned_message_idsthen returns the samemessage_idmore than once, and the duplicates consume theLIMIT, soa2a_mentions_feedreturns fewer distinct messages thanlimitrequests.
a2a_sendcallsrecord_mentionsonce per message today, so this is a latent risk. A UNIQUE index plusINSERT OR IGNOREmakes the write idempotent.♻️ Proposed fix
CREATE INDEX IF NOT EXISTS idx_mentions_handle_ts ON mentions(mentioned_handle, ts); + CREATE UNIQUE INDEX IF NOT EXISTS idx_mentions_unique + ON mentions(mentioned_handle, message_id);for handle in handles: self._conn.execute( - "INSERT INTO mentions (mentioned_handle, message_id, ts, thread) VALUES (?, ?, ?, ?)", + "INSERT OR IGNORE INTO mentions " + "(mentioned_handle, message_id, ts, thread) VALUES (?, ?, ?, ?)", (handle, message_id, ts, thread), )🤖 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/mentions.py` around lines 59 - 65, Add a UNIQUE constraint or unique index for the documented key `(mentioned_handle, message_id, ts, thread)` in the mentions schema, then update `record_mentions` to use conflict-safe insertion such as INSERT OR IGNORE. Preserve the existing commit behavior while making repeated calls idempotent and preventing duplicate IDs from limiting `get_mentioned_message_ids` results.tests/test_a2a_mentions.py (2)
85-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for an expired or malformed bearer token.
_make_tokennever sets anexpclaim. The suite covers three auth states on/a2a/mentions: no token (line 401), a valid token for the reader (line 406), and a valid token for a different subject (line 419).It does not cover an expired token or a token signed by an unknown key.
_handle_a2a_mentionsintaosmd/http_server.pymaps_ra.AuthErrorto 403 and a missing token to 401, and that 403 branch has no test. The docstring at lines 3-8 claims the file covers acceptance criterion 4 for authentication, so the gap matters.Add an
expparameter to_make_tokenand assert 403 for an expired token.🤖 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 `@tests/test_a2a_mentions.py` around lines 85 - 91, Add an optional exp parameter to _make_token and include it in the JWT claims when provided, then extend the /a2a/mentions authentication tests with an expired-token request and assert it returns 403, covering the _handle_a2a_mentions AuthError path while preserving existing token cases.
309-322: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
sincecursor test depends on wall-clock sleeps.The test writes a message, sleeps 20 ms, takes
pivot = time.time(), sleeps 20 ms, then writes a second message. It then asserts that only the second message appears.The mention
tscomes from the archive row timestamp, whicha2a_sendreads back at line 420 oftaosmd/service.py. On a loaded CI runner the twoa2a_sendcalls can take longer than the 20 ms margin, and clock resolution differences can place a timestamp on the wrong side ofpivot. The test then fails intermittently.Read the recorded timestamps and derive the pivot from them instead of from
time.time().🤖 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 `@tests/test_a2a_mentions.py` around lines 309 - 322, Update test_a2a_mentions_feed_since_cursor to derive pivot from the recorded archive timestamps of the first message rather than time.time() and wall-clock sleeps. Capture the timestamp associated with the “old `@bob`” send, then use a cursor value between that timestamp and the second message’s timestamp when calling a2a_mentions_feed, preserving the assertions that only “new `@bob`” is returned.taosmd/service.py (1)
419-423: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce the extra archive read and align the store guard.
Two points:
- Line 420 re-reads the row that line 405 just wrote, only to recover
timestamp. That is a second archive round trip on every send. Ifarchive.recordcan return the timestamp, or if the caller can generatets = time.time()before the write and pass it torecord, the read disappears.- Line 422-423 uses
stores.get("mentions")with anisinstanceguard, so a missing or wrong-typed store silently skips indexing.a2a_mentions_feedat line 676 usesstores["mentions"]and raisesKeyErrorin the same situation. Pick one behaviour. Silent skip on the write path plus a hard failure on the read path is the worst combination: mentions go unindexed with no signal.🤖 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/service.py` around lines 419 - 423, Remove the extra archive.get_event read in the send flow around archive.record by generating or obtaining the timestamp before the write and reusing it, or by consuming a timestamp returned from record. Align the mentions store access in this indexing path with a2a_mentions_feed: use the same strict missing/wrong-store behavior instead of silently skipping when the store is absent or has the wrong type.
🤖 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/http_server.py`:
- Around line 1548-1591: Update _handle_a2a_mentions to archive every request
and outcome with append-only Archive.record(...): record the incoming
query/request before validation or retrieval, authentication and validation
failures before returning or raising, and the retrieved response after a
successful feed call. Include user input, assistant output, tool/error details
as applicable, and do not delete or overwrite archived data.
In `@taosmd/mentions.py`:
- Line 15: Update the _MENTION_RE pattern to require either the start of the
string or a character that cannot be part of a handle immediately before @,
while preserving the existing handle characters and extraction behavior.
- Around line 67-78: Update MentionStore.get_mentioned_message_ids to clamp the
incoming limit to the supported non-negative maximum before adding it to the SQL
parameters, ensuring negative and excessively large values remain bounded while
preserving the existing query behavior.
In `@taosmd/remote.py`:
- Around line 251-269: Update a2a_mentions_feed to forward its reader argument
in the GET /a2a/mentions request parameters, while preserving the existing limit
and optional since parameters so standalone servers can authenticate the request
without a registry verifier.
In `@taosmd/service.py`:
- Around line 739-740: Remove the final result truncation after result.sort in
the mentions feed, keeping limit solely as the cap on mention rows applied
earlier in the flow. Return all messages produced by reply-chain expansion so a
requested mention page does not split an expanded thread.
- Around line 687-715: Refactor the A2A mention-chain logic in
_handle_a2a_mentions to avoid loading and rescanning the entire archive: narrow
archive.query using the mention rows when possible, parse each row once into a
reply_to-to-children map, and traverse that map from mentioned_ids in memory.
Derive each message’s thread root from the same parent/child relationships
instead of calling _find_thread_root or archive.get_event per hop, while
preserving the existing reply-chain and root results.
- Around line 779-785: The can_read path is hardcoded to always allow access,
leaving the mentionGrant logic unreachable. In taosmd/service.py lines 779-785,
replace the if True short-circuit with a named module constant such as
_CHANNEL_ACL_ALWAYS_ALLOWS and move _api._ensure_stores below that check so
stores are loaded only when needed. In tests/test_a2a_mentions.py lines 358-382,
add coverage with the constant disabled that asserts mentionGrant allows a
reader mentioned on the thread root and denies one absent from the chain; retain
the existing always-allow tests.
- Around line 719-740: Update a2a_mentions_feed to apply the same A2AAdminState
filtering as a2a_feed before appending messages: exclude rows whose IDs are in
_superseded and rows belonging to channels in _deleted, while preserving the
existing admin_action suppression. Reuse the established admin-state symbols and
filtering behavior so the mentions feed does not bypass administrative controls.
- Line 354: Validate recipient in a2a_send alongside sender and body, rejecting
non-string values before archive.record runs so invalid requests cannot
partially persist. Update the Returns documentation for a2a_send to include
recipient among the conditionally returned receipt fields.
In `@tests/test_a2a_mentions.py`:
- Around line 419-427: Update test_http_mentions_authenticated_as_other_excludes
to post a second message mentioning `@alice` after the existing `@bob` message, then
assert alice’s mentions response contains the `@alice` message and excludes the
`@bob` message. Keep the authenticated request and existing status validation
unchanged so the test verifies filtering against a non-empty feed.
- Around line 53-59: Update the store cleanup loop to iterate over every store
in each cached store mapping, including the claims store created by
_ensure_stores, rather than selecting named keys. Remove the broad exception
suppression around store.close() so close failures propagate or are reported
according to the test fixture’s existing cleanup behavior.
- Around line 401-403: Update test_http_mentions_unauthenticated_returns_401 to
assert the returned 401 error payload using body, verifying the handler’s
expected unauthenticated message. Keep the status assertion and remove the
unused-variable issue by making the payload assertion part of the test.
---
Nitpick comments:
In `@taosmd/mentions.py`:
- Around line 25-38: Update MentionStore.init to create the parent directory
before calling _db.connect, matching ClaimStore.init,
TemporalKnowledgeGraph.init, and ArchiveStore.init. Also add a
migrations.migrate call after the mentions executescript using a dedicated,
stable migration namespace for this store.
- Around line 59-65: Add a UNIQUE constraint or unique index for the documented
key `(mentioned_handle, message_id, ts, thread)` in the mentions schema, then
update `record_mentions` to use conflict-safe insertion such as INSERT OR
IGNORE. Preserve the existing commit behavior while making repeated calls
idempotent and preventing duplicate IDs from limiting
`get_mentioned_message_ids` results.
In `@taosmd/service.py`:
- Around line 419-423: Remove the extra archive.get_event read in the send flow
around archive.record by generating or obtaining the timestamp before the write
and reusing it, or by consuming a timestamp returned from record. Align the
mentions store access in this indexing path with a2a_mentions_feed: use the same
strict missing/wrong-store behavior instead of silently skipping when the store
is absent or has the wrong type.
In `@tests/test_a2a_mentions.py`:
- Around line 85-91: Add an optional exp parameter to _make_token and include it
in the JWT claims when provided, then extend the /a2a/mentions authentication
tests with an expired-token request and assert it returns 403, covering the
_handle_a2a_mentions AuthError path while preserving existing token cases.
- Around line 309-322: Update test_a2a_mentions_feed_since_cursor to derive
pivot from the recorded archive timestamps of the first message rather than
time.time() and wall-clock sleeps. Capture the timestamp associated with the
“old `@bob`” send, then use a cursor value between that timestamp and the second
message’s timestamp when calling a2a_mentions_feed, preserving the assertions
that only “new `@bob`” is returned.
🪄 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: 91c80ef4-f6c8-43cd-87e4-52726953b160
📒 Files selected for processing (6)
taosmd/api.pytaosmd/http_server.pytaosmd/mentions.pytaosmd/remote.pytaosmd/service.pytests/test_a2a_mentions.py
| def _handle_a2a_mentions(self, qs: dict) -> None: | ||
| since_raw = (qs.get("since") or [None])[0] | ||
| limit_raw = (qs.get("limit") or [50])[0] | ||
| try: | ||
| since = float(since_raw) if since_raw is not None else None | ||
| except (TypeError, ValueError) as exc: | ||
| raise _BadRequest("'since' must be a float timestamp") from exc | ||
| try: | ||
| limit_i = int(limit_raw) | ||
| except (TypeError, ValueError) as exc: | ||
| raise _BadRequest("'limit' must be an integer") from exc | ||
| # Auth: when a registry verifier is configured, the caller's | ||
| # verified identity is the reader. Unauthenticated requests | ||
| # return 401. When no verifier is configured (standalone), a | ||
| # ?reader= query parameter is accepted for testing. | ||
| if _registry_verifier is not None: | ||
| auth = self.headers.get("Authorization", "") | ||
| token = auth[len("Bearer "):].strip() if auth.startswith("Bearer ") else "" | ||
| if not token: | ||
| self._send_json(401, {"error": "registry auth: Bearer token required"}) | ||
| return | ||
| try: | ||
| from . import registry_auth as _ra # noqa: PLC0415 | ||
| import jwt as _jwt # noqa: PLC0415 | ||
| unverified = _jwt.decode(token, options={"verify_signature": False}) | ||
| raw_sub = unverified.get("sub", "") or "" | ||
| except Exception: # noqa: BLE001 | ||
| raw_sub = "" | ||
| try: | ||
| claims = _registry_verifier.authorize(token, raw_sub) | ||
| reader = claims.get("sub", "") | ||
| except _ra.AuthError as exc: | ||
| self._send_json(403, {"error": f"registry auth: {exc}"}) | ||
| return | ||
| else: | ||
| reader = (qs.get("reader") or [None])[0] | ||
| if not reader: | ||
| raise _BadRequest( | ||
| "'reader' query parameter is required when no registry verifier is configured" | ||
| ) | ||
| messages = runner.run( | ||
| service.a2a_mentions_feed(reader, since=since, limit=limit_i, data_dir=data_dir) | ||
| ) | ||
| self._send_json(200, {"messages": messages}) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Archive mention-feed requests and outcomes.
Record the request before retrieval. Record the response after retrieval. Record authentication and validation failures before returning. Use append-only Archive.record(...) and do not delete archived data.
As per coding guidelines, archive every conversation turn, including user messages, assistant responses, tool calls, and errors, using taOSmd’s append-only Archive.record(...); archived data must not be deleted.
🤖 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/http_server.py` around lines 1548 - 1591, Update _handle_a2a_mentions
to archive every request and outcome with append-only Archive.record(...):
record the incoming query/request before validation or retrieval, authentication
and validation failures before returning or raising, and the retrieved response
after a successful feed call. Include user input, assistant output, tool/error
details as applicable, and do not delete or overwrite archived data.
Source: Coding guidelines
|
|
||
| from taosmd import _db | ||
|
|
||
| _MENTION_RE = re.compile(r'@([a-zA-Z0-9_-]+)') |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add a left boundary to the mention pattern.
_MENTION_RE matches @handle at any position. It also matches inside an email address or a URL. For example, mail bob@alice.org records a mention for alice, and see http://x/@carol records a mention for carol. A mention is an access grant here: a2a_mentions_feed returns the message and its reply chain to the mentioned handle. So a stray @ in a body can expose a thread to an unintended agent.
Require the start of the string or a non-handle character before @.
🛡️ Proposed fix
-_MENTION_RE = re.compile(r'@([a-zA-Z0-9_-]+)')
+# Require start-of-string or whitespace/punctuation before "@" so that
+# emails ("bob@alice.org") and URLs do not create mention grants.
+_MENTION_RE = re.compile(r'(?:(?<=\s)|(?<=^))@([a-zA-Z0-9_-]+)', re.MULTILINE)🤖 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/mentions.py` at line 15, Update the _MENTION_RE pattern to require
either the start of the string or a character that cannot be part of a handle
immediately before @, while preserving the existing handle characters and
extraction behavior.
| async def get_mentioned_message_ids( | ||
| self, reader: str, since: float | None = None, limit: int = 50 | ||
| ) -> list[dict]: | ||
| query = "SELECT message_id, ts FROM mentions WHERE mentioned_handle = ?" | ||
| params: list = [reader] | ||
| if since is not None: | ||
| query += " AND ts > ?" | ||
| params.append(since) | ||
| query += " ORDER BY ts ASC LIMIT ?" | ||
| params.append(limit) | ||
| rows = self._conn.execute(query, params).fetchall() | ||
| return [{"message_id": r[0], "ts": r[1]} for r in rows] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find every caller of get_mentioned_message_ids and the limit values they pass.
rg -nP -C 5 '\bget_mentioned_message_ids\s*\(' --type=py
# Check how the HTTP layer validates the limit query parameter.
rg -nP -C 6 'limit_raw|_handle_a2a_mentions' --type=pyRepository: jaylfc/taosmd
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -u
echo "Tracked files matching mentions/http:"
git ls-files | rg '(^|/)mentions\.py$|(^|/)http_server\.py$|(^|/)a2a_feed\.py$' || true
echo
echo "Find get_mentioned_message_ids with fixed string (no type filter):"
rg -n -C 5 'get_mentioned_message_ids' . || true
echo
echo "Find HTTP mention handlers (no type filter):"
rg -n -C 6 'handle.*mention|a2a/mentions|limit_raw|_handle_a2a_mentions|mentions' . || trueRepository: jaylfc/taosmd
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -u
echo "taosmd/mentions.py:"
cat -n taosmd/mentions.py
echo
echo "taosmd/http_server.py around mention endpoints and route parsing:"
rg -n -C 8 'a2a/mentions|handle_a2a|http.route|route\(|query\(|limit' taosmd/http_server.py taosmd/*.py | sed -n '1,260p'
echo
echo "service.py a2a_mentions_feed block:"
sed -n '651,705p' taosmd/service.py | cat -n
echo
echo "remote.py a2a_mentions_feed block:"
sed -n '251,269p' taosmd/remote.py | cat -n
echo
echo "Tests around a2a_mentions_feed and limit:"
rg -n -C 5 'a2a_mentions_feed|limit|mentions' tests/test_a2a_mentions.py taosmd/tests 2>/dev/null || trueRepository: jaylfc/taosmd
Length of output: 49562
🏁 Script executed:
#!/bin/bash
set -u
echo "taosmd/http_server.py size and relevant mentions route:"
wc -l taosmd/http_server.py
rg -n -C 10 '/a2a/mentions|make_server|query_string|parse|service_loop|GET|routes' taosmd/http_server.py
echo
echo "Focused sections around line numbers:"
sed -n '1,220p' taosmd/http_server.py | cat -nRepository: jaylfc/taosmd
Length of output: 47982
🏁 Script executed:
#!/bin/bash
set -u
echo "_handle_a2a_mentions implementation:"
rg -n -C 35 'def _handle_a2a_mentions' taosmd/http_server.py
echo
echo "Parse-query helper locations:"
rg -n -C 5 'def .*parse|parse_qs|qs\.get\(.*since|qs\.get\(.*limit|limit_raw|since_raw' taosmd/http_server.pyRepository: jaylfc/taosmd
Length of output: 12421
🏁 Script executed:
#!/bin/bash
set -u
echo "_handle_a2a_mentions remainder:"
sed -n '1548,1598p' taosmd/http_server.py | cat -n
echo
echo "service.a2a_mentions_feed remainder:"
sed -n '699,735p' taosmd/service.py | cat -nRepository: jaylfc/taosmd
Length of output: 4860
Clamp limit at the store boundary.
get_mentioned_message_ids passes limit directly into LIMIT ?, and the HTTP handler only parses ?limit= as an integer. A negative LIMIT disables the limit, and a large positive value removes the intended cap. Clamp limit to the supported non-negative range in MentionStore so each call remains bounded.
🤖 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/mentions.py` around lines 67 - 78, Update
MentionStore.get_mentioned_message_ids to clamp the incoming limit to the
supported non-negative maximum before adding it to the SQL parameters, ensuring
negative and excessively large values remain bounded while preserving the
existing query behavior.
| async def a2a_mentions_feed( | ||
| self, | ||
| reader: str, | ||
| *, | ||
| since: float | None = None, | ||
| limit: int = 50, | ||
| **_opts, | ||
| ) -> list[dict]: | ||
| """GET /a2a/mentions: return messages mentioning ``reader`` plus reply chains. | ||
|
|
||
| Returns the ``messages`` list from the server response. Requires | ||
| registry auth on the server side; the ``reader`` identity is derived | ||
| from the verified token ``sub``. | ||
| """ | ||
| params: dict = {"limit": limit} | ||
| if since is not None: | ||
| params["since"] = since | ||
| resp = await self._run("GET", "/a2a/mentions", params=params) | ||
| return resp.get("messages", []) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Forward reader for standalone servers.
Line 253 accepts reader, but Line 268 drops it. When no registry verifier is configured, taosmd/http_server.py requires ?reader=. Every remote mention-feed request to a standalone server returns 400.
Proposed fix
- params: dict = {"limit": limit}
+ params: dict = {"reader": reader, "limit": limit}📝 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.
| async def a2a_mentions_feed( | |
| self, | |
| reader: str, | |
| *, | |
| since: float | None = None, | |
| limit: int = 50, | |
| **_opts, | |
| ) -> list[dict]: | |
| """GET /a2a/mentions: return messages mentioning ``reader`` plus reply chains. | |
| Returns the ``messages`` list from the server response. Requires | |
| registry auth on the server side; the ``reader`` identity is derived | |
| from the verified token ``sub``. | |
| """ | |
| params: dict = {"limit": limit} | |
| if since is not None: | |
| params["since"] = since | |
| resp = await self._run("GET", "/a2a/mentions", params=params) | |
| return resp.get("messages", []) | |
| async def a2a_mentions_feed( | |
| self, | |
| reader: str, | |
| *, | |
| since: float | None = None, | |
| limit: int = 50, | |
| **_opts, | |
| ) -> list[dict]: | |
| """GET /a2a/mentions: return messages mentioning ``reader`` plus reply chains. | |
| Returns the ``messages`` list from the server response. Requires | |
| registry auth on the server side; the ``reader`` identity is derived | |
| from the verified token ``sub``. | |
| """ | |
| params: dict = {"reader": reader, "limit": limit} | |
| if since is not None: | |
| params["since"] = since | |
| resp = await self._run("GET", "/a2a/mentions", params=params) | |
| return resp.get("messages", []) |
🤖 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/remote.py` around lines 251 - 269, Update a2a_mentions_feed to forward
its reader argument in the GET /a2a/mentions request parameters, while
preserving the existing limit and optional since parameters so standalone
servers can authenticate the request without a registry verifier.
| reply_to: str | None = None, | ||
| refs: list | None = None, | ||
| blocks: list | None = None, | ||
| recipient: str | None = None, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate recipient like sender and body.
a2a_send validates sender and body as non-empty strings at lines 380-383. recipient gets no check. The HTTP /a2a/send handler forwards JSON body values, so recipient can arrive as a list, a dict, or a number.
The failure is order-dependent. archive.record at line 405 runs first and succeeds. Then record_mentions calls handles.add(recipient). For an unhashable value such as a list this raises TypeError. The request returns an error, but the message is already in the append-only archive without a mention row. The archive and the mention index then disagree.
Reject a non-string recipient up front.
🛡️ Proposed fix
if not isinstance(body, str) or not body:
raise ValueError("body must be a non-empty string")
+ if recipient is not None and (not isinstance(recipient, str) or not recipient):
+ raise ValueError("recipient must be a non-empty string when provided")Also update the "Returns" paragraph at lines 374-375. It lists refs and blocks as the conditional receipt fields but omits recipient, which lines 413-414 now add.
🤖 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/service.py` at line 354, Validate recipient in a2a_send alongside
sender and body, rejecting non-string values before archive.record runs so
invalid requests cannot partially persist. Update the Returns documentation for
a2a_send to include recipient among the conditionally returned receipt fields.
| result.sort(key=lambda m: m["ts"]) | ||
| return result[:limit] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
limit is applied twice with different meanings.
Line 679 uses limit to cap the number of mention rows. Line 740 uses the same limit to cap the final result, after the reply chain expanded the set.
The two caps interact badly. Consider limit=3 with one mentioned root that has five replies. Line 679 returns one mention row. The expansion adds five replies, giving six messages. Line 739 sorts by ts, and line 740 keeps the three oldest. A reader who asks for a page of mentions gets a partial thread with no cursor to continue from.
test_a2a_mentions_feed_limit asserts only len(msgs) == 3, so it passes either way.
Decide which one limit means. If it caps mentions, drop the truncation at line 740. If it caps returned messages, document that a thread can be split across pages and return a cursor.
🤖 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/service.py` around lines 739 - 740, Remove the final result truncation
after result.sort in the mentions feed, keeping limit solely as the cap on
mention rows applied earlier in the flow. Return all messages produced by
reply-chain expansion so a requested mention page does not split an expanded
thread.
| stores = await _api._ensure_stores(data_dir) | ||
| archive = stores["archive"] | ||
| mentions_store = stores["mentions"] | ||
|
|
||
| # channelACL: no enforcement yet (tsk-dp6fyv); always passes. | ||
| if True: | ||
| return True |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
can_read is stubbed to always return True, so both of its tests pass without exercising any logic. The if True: short-circuit at taosmd/service.py line 784 makes lines 787-819 unreachable. Every can_read call returns True. The two tests then assert True for the granted case and for the non-granted case, so neither can fail and neither covers the mentionGrant walk.
taosmd/service.py#L779-L785: replaceif True:with a named module constant such as_CHANNEL_ACL_ALWAYS_ALLOWS, and move the_ensure_storescall below the short-circuit so the unusedarchiveandmentions_storeloads disappear.tests/test_a2a_mentions.py#L358-L382: add a test that sets the constant toFalseand drives the mentionGrant walk directly. AssertTruefor a reader mentioned on the thread root andFalsefor a reader mentioned nowhere in the chain. Keep the existing two tests as the always-allow baseline.
📍 Affects 2 files
taosmd/service.py#L779-L785(this comment)tests/test_a2a_mentions.py#L358-L382
🤖 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/service.py` around lines 779 - 785, The can_read path is hardcoded to
always allow access, leaving the mentionGrant logic unreachable. In
taosmd/service.py lines 779-785, replace the if True short-circuit with a named
module constant such as _CHANNEL_ACL_ALWAYS_ALLOWS and move _api._ensure_stores
below that check so stores are loaded only when needed. In
tests/test_a2a_mentions.py lines 358-382, add coverage with the constant
disabled that asserts mentionGrant allows a reader mentioned on the thread root
and denies one absent from the chain; retain the existing always-allow tests.
| for stores in list(taosmd_api._stores_cache.values()): | ||
| for store in (stores.get("archive"), stores.get("vector"), stores.get("kg"), stores.get("mentions")): | ||
| if store and hasattr(store, "close"): | ||
| try: | ||
| asyncio.run(store.close()) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Close the claims store too, and do not swallow close errors silently.
Two points:
- Line 54 closes
archive,vector,kg, andmentions._ensure_storesintaosmd/api.pyalso creates aClaimStoreat line 145 and holds its SQLite connection. The fixture never closes it, so each test in this file leaks one open handle against atmp_pathdatabase. - Lines 56-59 catch every exception and pass. Ruff flags this as S110 and BLE001. A failed
close()produces no signal, so a leak or a lock error stays invisible.
Iterate the store values instead of naming a subset. That way a new store added to _ensure_stores is closed automatically.
♻️ Proposed fix
for stores in list(taosmd_api._stores_cache.values()):
- for store in (stores.get("archive"), stores.get("vector"), stores.get("kg"), stores.get("mentions")):
- if store and hasattr(store, "close"):
- try:
- asyncio.run(store.close())
- except Exception:
- pass
+ for name, store in stores.items():
+ if not hasattr(store, "close"):
+ continue
+ try:
+ asyncio.run(store.close())
+ except OSError as exc: # noqa: PERF203
+ print(f"failed to close store {name}: {exc}")📝 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.
| for stores in list(taosmd_api._stores_cache.values()): | |
| for store in (stores.get("archive"), stores.get("vector"), stores.get("kg"), stores.get("mentions")): | |
| if store and hasattr(store, "close"): | |
| try: | |
| asyncio.run(store.close()) | |
| except Exception: | |
| pass | |
| for stores in list(taosmd_api._stores_cache.values()): | |
| for name, store in stores.items(): | |
| if not hasattr(store, "close"): | |
| continue | |
| try: | |
| asyncio.run(store.close()) | |
| except OSError as exc: # noqa: PERF203 | |
| print(f"failed to close store {name}: {exc}") |
🧰 Tools
🪛 Ruff (0.16.0)
[error] 58-59: try-except-pass detected, consider logging the exception
(S110)
[warning] 58-58: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@tests/test_a2a_mentions.py` around lines 53 - 59, Update the store cleanup
loop to iterate over every store in each cached store mapping, including the
claims store created by _ensure_stores, rather than selecting named keys. Remove
the broad exception suppression around store.close() so close failures propagate
or are reported according to the test fixture’s existing cleanup behavior.
Source: Linters/SAST tools
| def test_http_mentions_unauthenticated_returns_401(authed_server): | ||
| status, body = _get(f"{authed_server}/a2a/mentions") | ||
| assert status == 401 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the 401 error payload and remove the unused variable.
Line 402 unpacks body and never uses it. Ruff reports this as RUF059. The handler returns a specific message for the unauthenticated case, so asserting it costs one line and makes the test verify the contract rather than only the status code.
💚 Proposed fix
def test_http_mentions_unauthenticated_returns_401(authed_server):
status, body = _get(f"{authed_server}/a2a/mentions")
- assert status == 401
+ assert status == 401, body
+ assert "Bearer token required" in body["error"]📝 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 test_http_mentions_unauthenticated_returns_401(authed_server): | |
| status, body = _get(f"{authed_server}/a2a/mentions") | |
| assert status == 401 | |
| def test_http_mentions_unauthenticated_returns_401(authed_server): | |
| status, body = _get(f"{authed_server}/a2a/mentions") | |
| assert status == 401, body | |
| assert "Bearer token required" in body["error"] |
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 402-402: Unpacked variable body is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 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 `@tests/test_a2a_mentions.py` around lines 401 - 403, Update
test_http_mentions_unauthenticated_returns_401 to assert the returned 401 error
payload using body, verifying the handler’s expected unauthenticated message.
Keep the status assertion and remove the unused-variable issue by making the
payload assertion part of the test.
Source: Linters/SAST tools
| def test_http_mentions_authenticated_as_other_excludes(authed_server): | ||
| alice_token = _make_token("alice", iss=REGISTRY_ISS) | ||
| _post(f"{authed_server}/a2a/send", | ||
| {"from": "agentA", "body": "hey @bob", "thread": "cross"}, token=alice_token) | ||
|
|
||
| status, body = _get(f"{authed_server}/a2a/mentions", token=alice_token) | ||
| assert status == 200, body | ||
| msgs = body["messages"] | ||
| assert all("hey @bob" not in m.get("body", "") for m in msgs) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This test passes trivially and does not prove exclusion.
alice posts a message that mentions @bob. alice then reads her own mention feed. alice is never mentioned anywhere in this test, so her feed is empty. The assertion at line 427 holds over an empty list regardless of whether the filter works.
Make the test meaningful: mention @alice in a second message, then assert her feed contains that message and not the @bob one. The feed then has content, and the assertion tests the handle filter.
💚 Proposed fix
def test_http_mentions_authenticated_as_other_excludes(authed_server):
alice_token = _make_token("alice", iss=REGISTRY_ISS)
_post(f"{authed_server}/a2a/send",
{"from": "agentA", "body": "hey `@bob`", "thread": "cross"}, token=alice_token)
+ _post(f"{authed_server}/a2a/send",
+ {"from": "agentA", "body": "hey `@alice`", "thread": "cross"}, token=alice_token)
status, body = _get(f"{authed_server}/a2a/mentions", token=alice_token)
assert status == 200, body
msgs = body["messages"]
+ bodies = [m["body"] for m in msgs]
+ assert "hey `@alice`" in bodies
- assert all("hey `@bob`" not in m.get("body", "") for m in msgs)
+ assert "hey `@bob`" not in bodies📝 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 test_http_mentions_authenticated_as_other_excludes(authed_server): | |
| alice_token = _make_token("alice", iss=REGISTRY_ISS) | |
| _post(f"{authed_server}/a2a/send", | |
| {"from": "agentA", "body": "hey @bob", "thread": "cross"}, token=alice_token) | |
| status, body = _get(f"{authed_server}/a2a/mentions", token=alice_token) | |
| assert status == 200, body | |
| msgs = body["messages"] | |
| assert all("hey @bob" not in m.get("body", "") for m in msgs) | |
| def test_http_mentions_authenticated_as_other_excludes(authed_server): | |
| alice_token = _make_token("alice", iss=REGISTRY_ISS) | |
| _post(f"{authed_server}/a2a/send", | |
| {"from": "agentA", "body": "hey `@bob`", "thread": "cross"}, token=alice_token) | |
| _post(f"{authed_server}/a2a/send", | |
| {"from": "agentA", "body": "hey `@alice`", "thread": "cross"}, token=alice_token) | |
| status, body = _get(f"{authed_server}/a2a/mentions", token=alice_token) | |
| assert status == 200, body | |
| msgs = body["messages"] | |
| bodies = [m["body"] for m in msgs] | |
| assert "hey `@alice`" in bodies | |
| assert "hey `@bob`" not in bodies |
🤖 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 `@tests/test_a2a_mentions.py` around lines 419 - 427, Update
test_http_mentions_authenticated_as_other_excludes to post a second message
mentioning `@alice` after the existing `@bob` message, then assert alice’s mentions
response contains the `@alice` message and excludes the `@bob` message. Keep the
authenticated request and existing status validation unchanged so the test
verifies filtering against a non-empty feed.
Code Review by Qodo
Context used✅ Compliance rules (platform):
13 rules 1. Mentions feed drops old
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| all_rows = await archive.query(event_type=EVENT_A2A, limit=100_000) | ||
| reply_chain_ids = set(mentioned_ids) |
There was a problem hiding this comment.
1. Mentions feed drops old 🐞 Bug ≡ Correctness
service.a2a_mentions_feed only scans the most recent 100,000 A2A archive rows, so mentions that point to older message_ids will never be returned and reply chains can be incomplete on long-running installs. This makes /a2a/mentions silently incorrect once the A2A archive exceeds that window.
Agent Prompt
### Issue description
`a2a_mentions_feed()` limits its archive scan to `limit=100_000`, which makes the mentions endpoint incomplete when the archive contains more than 100k A2A rows.
### Issue Context
The mention index (`MentionStore`) can return `message_id`s that are older than the newest 100k archive rows; the feed then only emits rows present in the truncated `all_rows` list.
### Fix Focus Areas
- taosmd/service.py[651-741]
- taosmd/archive.py[287-328]
- taosmd/mentions.py[67-78]
### Suggested fix
- Remove the fixed `100_000` cap and instead **page** through `archive.query(..., limit=batch, offset=...)` until you’ve loaded enough history to include the oldest relevant mention timestamp (e.g., `min(r["ts"])` from `mentioned_rows`) plus any additional rows needed to resolve reply chains.
- Alternatively, add an archive helper to fetch specific IDs (and potentially their descendants) so the endpoint doesn’t rely on a global “latest N rows” scan.
- Add a regression test that simulates an archive with >100k A2A events where a mention points to an older message and verify it is still returned.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| query += " ORDER BY ts ASC LIMIT ?" | ||
| params.append(limit) |
There was a problem hiding this comment.
3. Mentions page selects oldest 🐞 Bug ≡ Correctness
MentionStore.get_mentioned_message_ids always uses ORDER BY ts ASC LIMIT ?, so when since is omitted the first /a2a/mentions page returns the oldest mentions rather than the most recent ones. Unlike a2a_feed (which explicitly limits the most-recent N when since is None), this makes it hard for clients to efficiently fetch current mentions once a reader has more than limit mentions.
Agent Prompt
### Issue description
The mentions index query always returns the oldest rows first (`ORDER BY ts ASC LIMIT ?`), which makes the default initial page (`since=None`) return stale data once there are more than `limit` mentions.
### Issue Context
`a2a_feed()` documents/implements a different behavior: when `since` is `None`, it limits the most recent N rows (newest-first query then reverse to chronological).
### Fix Focus Areas
- taosmd/mentions.py[67-78]
- taosmd/service.py[434-498]
### Suggested fix
- Adjust `get_mentioned_message_ids()` to mirror `a2a_feed` semantics:
- If `since is None`: `ORDER BY ts DESC LIMIT ?` (select most recent page) and reverse in Python before returning to preserve oldest-first output ordering.
- If `since is not None`: keep `ORDER BY ts ASC LIMIT ?` (cursor-forward paging).
- Add a test where a reader has >limit mentions and verify that `since=None, limit=N` returns the most recent N (in ascending order for display).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| return await remote.a2a_send( | ||
| sender, body, thread=thread, reply_to=reply_to, | ||
| refs=refs, blocks=blocks, | ||
| refs=refs, blocks=blocks, recipient=recipient, |
There was a problem hiding this comment.
CRITICAL: remote.a2a_send swallows recipient via **_opts
service.a2a_send now passes recipient=recipient to remote.a2a_send, but RemoteClient.a2a_send accepts **_opts and silently drops the recipient kwarg without forwarding it to the server. Remote deployments lose the recipient field entirely, so the new mentions feature is broken for any caller using TAOSMD_SERVER_URL.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| params: dict = {"limit": limit} | ||
| if since is not None: | ||
| params["since"] = since | ||
| resp = await self._run("GET", "/a2a/mentions", params=params) |
There was a problem hiding this comment.
WARNING: a2a_mentions_feed accepts reader but never forwards it
RemoteClient.a2a_mentions_feed takes a reader parameter but does not include it in the GET request parameters. The server derives reader from the authenticated token's sub claim, so the parameter is silently ignored. This makes the client method signature misleading — callers may expect to query mentions for an arbitrary user, but they can only query for the token owner.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 186.9K · Output: 34.3K · Cached: 2.1M |
|
FIX, and the good news first because it is the part that matters: the index is REAL. mentions.py has a proper (handle, ts) index and the query is a covered range scan, measured flat at 0.31-0.35ms whether the archive holds 10k or 50k rows. Crucially the early return fires BEFORE any scan, so the steady-state 'anything for me?' poll is 0.39ms flat. That is exactly the cheap-poll primitive the zero-token watcher needs, and it is delivered. Also: 21 tests, they run, and two of three mutations go red. Best-shaped taosmd PR today. The cost mechanism is half-built though: free to ask, whole-archive to answer. BLOCKING:
Adjudication of the bots: CodeRabbit was largely right and found six of these independently. Its recipient finding has the wrong mechanism (it assumed HTTP wiring exists, which is itself evidence of blocker 3) but a useful conclusion. REJECT qodo's suggestion to flip ORDER BY ts ASC to DESC: ASC is correct and load-bearing, it is what makes the watermark gapless, and I verified exactly-once delivery over a 9-mention interleaved chain. Fix the cold-start concern with a documented cursor, never by reversing the sort. Kilo never completed here, so treat it as unadjudicated rather than clean. NON-BLOCKING but worth doing while you are in there: return next_since so clients do not have to infer the watermark; tiebreak the cursor on (ts, id), since equal timestamps silently drop a row at a page boundary and a backfill with coarse timestamps would hit that immediately; clamp limit and reject negatives (a negative becomes LIMIT -1, unlimited); expose the feed on MCP alongside a2a_read. Retitle please, seventh in a row wearing the uv.lock cleanup subject. |
1 participant
Autonomous build of board card tsk-lmlx2v.
Files:
taosmd/api.py | 4 +
taosmd/http_server.py | 50 ++++-
taosmd/mentions.py | 85 ++++++++
taosmd/remote.py | 20 ++
taosmd/service.py | 197 ++++++++++++++++-
tests/test_a2a_mentions.py | 512 +++++++++++++++++++++++++++++++++++++++++++++
6 files changed, 866 insertions(+), 2 deletions(-)
Summary by CodeRabbit