Revise PR #233: mention feed - send reader on the remote path, normalise handles, and stop the cross-channel mention leak

[jaylfc]

CARD TITLE (intent, not commit subject): Revise PR #233: mention feed - send reader on the remote path, normalise handles, and stop the cross-channel mention leak

Autonomous build of board card tsk-6icvd4.

Files:
changelog.d/tsk-6icvd4-mentions-feed.md | 7 +
taosmd/api.py | 4 +
taosmd/http_server.py | 53 ++-
taosmd/mentions.py | 92 ++++++
taosmd/remote.py | 20 ++
taosmd/service.py | 168 +++++++++-
tests/test_a2a_mentions.py | 564 ++++++++++++++++++++++++++++++++
7 files changed, 906 insertions(+), 2 deletions(-)

[qodo-code-review]

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

[coderabbitai]

Warning

Review limit reached

@jaylfc, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 32 minutes

Limit details: You’ve used all 1 included review currently available under your plan.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 71de0295-bb0d-4ef9-a89b-b19fa59ef168

📥 Commits

Reviewing files that changed from the base of the PR and between fc44ec6 and c2218c7.

📒 Files selected for processing (7)

• changelog.d/tsk-6icvd4-mentions-feed.md
• taosmd/api.py
• taosmd/http_server.py
• taosmd/mentions.py
• taosmd/remote.py
• taosmd/service.py
• tests/test_a2a_mentions.py

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[gitar-bot]

Important

You are using the Gitar free plan. Upgrade to unlock code review, CI analysis, auto-apply, custom automations, and more.

_Gitar

[kilo-code-bot]

CRITICAL: reader = qp_reader or token_sub allows ?reader= to override the authenticated token identity

When a registry verifier is configured, the server authenticates the Bearer token and extracts token_sub, but then silently replaces it with any ?reader= query parameter supplied by the caller. Any valid-token holder can query any other user's mentions simply by changing that parameter — e.g. GET /a2a/mentions?reader=alice with a bob token returns Alice's mentions. The authorize() call validates the token's validity, not that token_sub matches qp_reader.

Remove the qp_reader override when a verifier is configured; reader must always be token_sub.

---

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

[kilo-code-bot]

WARNING: Bare except Exception on jwt.decode silently masks all JWT parsing errors

jwt.decode() can raise jwt.exceptions.DecodeError, jwt.exceptions.InvalidTokenError, and other specific subclasses for malformed, expired, or algorithm-mismatched tokens. Catching Exception collapses all of these into raw_sub = "", which is then forwarded to authorize(). While authorize() performs its own full verification, the empty-string raw_sub will produce an incorrect or confusing error if the verifier logs or includes it in an error message, and makes it harder to distinguish between a malformed token and a legitimate one whose sub extraction failed for some other reason.

Catch jwt.exceptions.PyJWTError specifically (or Exception only as a final fallback with a logged warning), so genuine JWT format errors are distinguishable.

---

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

[kilo-code-bot]

WARNING: root_threads is computed but never referenced — dead code

root_threads = {msg_thread[mid] for mid in mentioned_ids if mid in msg_thread}

This set comprehension is evaluated but root_threads is never used anywhere in the function. The actual thread-root data is correctly built via thread_roots through _find_thread_root (lines 954–958), so root_threads appears to be a leftover from a refactor. Dead code of this form can mask incomplete logic or confuse future maintainers about which data structure is authoritative.

Remove root_threads = ... or document why it is intentionally unused.

---

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

[kilo-code-bot]

WARNING: a2a_mentions_feed unconditionally sends reader as a query parameter, enabling server-side auth bypass

The remote client always forwards reader in the request params. Combined with the server-side behaviour at http_server.py:1699 (reader = qp_reader or token_sub), any caller that can control the reader argument to the remote client can impersonate any other user's identity when calling this endpoint against a registry-authed server.

Document this risk explicitly, or — better — make the server ignore ?reader= when a verifier is configured, removing the exposure regardless of which client is used.

---

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

[kilo-code-bot]

Code Review Summary

Status: 4 Issues Found | Recommendation: Address before merge

Overview

Severity	Count
CRITICAL	1
WARNING	3

Issue Details (click to expand)

CRITICAL

File	Line	Issue
taosmd/http_server.py	1699	reader = qp_reader or token_sub allows ?reader= query param to override authenticated token identity — any valid-token holder can query any user's mentions

WARNING

File	Line	Issue
taosmd/http_server.py	1690	Bare except Exception on jwt.decode silently swallows all JWT parsing errors, masking malformed or invalid tokens
taosmd/service.py	940	root_threads computed but never referenced — dead code from an incomplete refactor
taosmd/remote.py	265	a2a_mentions_feed always forwards reader as a query param; combined with server behaviour at http_server.py:1699 this is the vehicle for the auth bypass

Files Reviewed (5 files)

• taosmd/http_server.py — 2 issues
• taosmd/service.py — 1 issue
• taosmd/remote.py — 1 issue
• taosmd/mentions.py
• taosmd/api.py
• tests/test_a2a_mentions.py

Fix these issues in Kilo Cloud

---

_{Reviewed by step-3.7-flash · Input: 93.1K · Output: 9.1K · Cached: 622.3K}

[jaylfc]

Review: BLOCK

Reviewed at c2218c7, trial-merged with master 0cc66e6. The carry-forward is real and three of the card's items are genuinely fixed and I reproduced each of them. Two things block: one is a cross-identity read that this branch introduces, and one silently truncates the feature to the newest 50 messages on the bus. Both are measured below with the pre-fix tree as the control.

Suite on the trial merge: 1504 passed, 12 skipped (master baseline 1479/12, +25 = the 25 tests in tests/test_a2a_mentions.py). Conflict-marker grep, check_deleted_symbols.py --base origin/master and normalise_handle_gate.py all clean, exit 0, run on the merged tree. git rev-list --merges over the branch is empty, so acceptance 6 passes and the --squash instruction was followed.

---

BLOCKER 1 — ?reader= now overrides the verified token identity

http_server.py:1699

qp_reader = (qs.get("reader") or [None])[0]
reader = qp_reader or token_sub      # the query parameter wins

Any holder of any valid registry token can read any other agent's mention feed. Same request, same token, the only difference is the query parameter, so the two arms are the finding:

carol's valid token, GET /a2a/mentions              -> 200 ['note for @carol']
carol's valid token, GET /a2a/mentions?reader=bob   -> 200 ['secret for @bob']
no token,            GET /a2a/mentions?reader=bob   -> 401

Reachable through the public client too, not only by hand-built curl. RemoteClient(token=carol).a2a_mentions_feed("bob") returns ['secret for @bob'].

This is a regression, not an inherited defect. #233 exec/tsk-lmlx2v at the same line used the verified subject and nothing else (reader = claims.get("sub", "")). The same probe on that tree:

                                      #233            #320
carol token, no reader param      note for @carol   note for @carol
carol token, ?reader=bob          note for @carol   secret for @bob   <-- introduced here

#233's bug was that the client never sent reader, so a carol install asking for bob got carol's data — wrong answer, no disclosure. Sending reader was the right fix; honouring it at the server without checking it against the verified subject converted a wrong-answer bug into a cross-identity read. The feed is exactly the wrong place for it: it returns the mentioned message plus its whole reply chain regardless of channel, and can_read is return True, so there is no second gate behind it.

I want to be straight about where this came from: the card's acceptance item 1 invited it. It reads "a carol-token install asking for bob's mentions gets BOB's or a clear error, never carol's silently", which taken literally at the server is what got built. That wording is a card defect and it is being corrected on the revision card, not charged to this build. The shape that was wanted:

• client always sends reader (this branch does this correctly, keep it);
• server, when a verifier is configured, treats the verified sub as authoritative and fails closed with 403 when reader is present and does not normalise to it — never the reuse arm;
• ?reader= stays free only on the no-verifier standalone path, where it already is.

No test covers the arm that broke. test_http_mentions_authenticated_as_other_excludes sends alice's token with no reader parameter, so it only ever exercises the token_sub fallback. The escalation path has no test at all, and neither does RemoteClient.a2a_mentions_feed, which acceptance 1 explicitly asked for.

BLOCKER 2 — the feed goes blind past 50 messages on the bus

service.py:922

all_rows = await archive.query(event_type=EVENT_A2A)     # limit defaults to 50, ORDER BY timestamp DESC

#233 called this same query with limit=100_000. Dropping the argument does not remove the ceiling, it lowers it to archive.query's default of 50, newest first. A mention older than the newest 50 messages on the bus is filtered out of all_rows and the endpoint answers 200 {"messages": []}.

One mention posted first, then N-1 unrelated messages, same probe on both trees:

bus size     10   40   49   51   60   100   500   3000
#320 (this)   1    1    1    0    0     0     0      0     <-- cliff at 50
#233          1    1    1    1    1     1     1      1

The live build channel is at 3108 messages, so on the real bus this feature returns empty for every mention older than about the last hour. It is silent — 200, empty list, no error.

It also accounts for most of the reported speed-up. The changelog line "quadratic reply-chain traversal replaced with an O(n) adjacency-list walk" is measuring a scan of 50 rows against #233's scan of everything. The adjacency-list rewrite is a genuine improvement and worth keeping; it needs measuring again with the limit restored. Note that this line is the only A2A archive read in service.py without an explicit limit — the five others (:620, :699, :733, :818 and :545) all pass one. The card asked for the silent 100k ceiling to be fixed; this replaces it with a silent 50.

---

Card items that are genuinely fixed — reproduced, keep all of it

• Cross-channel leak (blocker 3). The child_thread == parent_thread check in the chain walk is correct. Verified the leak case is excluded and the sibling-exclusion control from tsk-lmlx2v [OPEN] A2A mention index + feed: reach an agent in a chan #233 still passes.
• Handle normalisation (blocker 2). bob / @bob / BOB resolve to one identity, applied on write and on read. _normalise_handle is now the only handle-normalising expression in taosmd/ — this branch effectively lands the shared helper that tsk-pgtl4b was closed without landing, so the dependency this card declares was never satisfiable and the lane did the right thing anyway. See collections: revoke could not remove a grant its own grant call had stored #315 for the same one-helper-both-ends shape.
• Mention regex. Measured: ops@bob.example.com -> no match, https://site.test/@carol/page -> no match, both fixed. Quoted forms ("@dave", `@dave`, > @dave, inside a fenced block) still match; that third case from the card is unaddressed, and it is arguable a quoted mention is still a mention, so it is a note rather than a defect.
• Carry-forward. All of tsk-lmlx2v [OPEN] A2A mention index + feed: reach an agent in a chan #233's tests are present plus 4 new (25 total), and its sibling-exclusion property still holds. This was checked against the branch's own base, not against master.

Remaining defects, all measured

1. limit is still applied twice — service.py:915 caps the mention query and :982 re-caps the joined result. One mention with 5 chained replies, limit=3, returns ['root for @bob', 'reply 0', 'reply 1']: one mention plus two chain replies, which is verbatim the symptom card defect 3 describes. The changelog claims "limit is applied once".
2. limit=100000000000000000000 returns 500 (sqlite OverflowError). 1e20, -1, 0, nan, inf all correctly return 400, so acceptance 4 is close but the integer-literal form still 500s.
3. can_read is still a stub — service.py:1010 is return True, still exported in __all__, still zero callers in taosmd/. The ~30 dead lines were removed, which is real, but the card said remove or implement and neither happened. Its two tests assert True and pass against an empty body.
4. root_threads (service.py:940) is computed and never read.
5. import math in mentions.py:12 is unused.
6. http_server.py:113 adds a second, less complete GET /a2a/stream row to the endpoint table, next to the new /a2a/mentions row.
7. The card asked the body to say this supersedes tsk-lmlx2v [OPEN] A2A mention index + feed: reach an agent in a chan #233; it does not, and it reports no suite count.

Kilo independently flagged blocker 1 and the dead root_threads. I had measured both before reading its comment; recording that because a bot finding is a claim like any other and this one was right.

---

Closing under the close-on-block policy, with a revision card to follow that carries forward the verified-good work above so it is not rebuilt, and states the fail-closed patch shape for blocker 1 explicitly so the card's own wording cannot invite the reuse arm a second time. Branch exec/tsk-6icvd4 is preserved.

[jaylfc]

Revision card is tsk-wlq4ui — Revise PR #320: fail closed when ?reader= disagrees with the verified token, and restore the archive limit the mention feed lost. It carries forward everything verified good above (the cross-channel fix, the normalisation helper, the regex fix, the client-side reader, the adjacency-list walk and all 25 tests) so none of it is rebuilt, and it states the fail-closed patch shape explicitly.

Correction to the record: the close reason on the executing card tsk-6icvd4 cites the revision card as tsk-1cpvk9. That id does not exist — I wrote it before the card was created and ids are server-assigned. The real id is tsk-wlq4ui. The board rejects both a PATCH (403) and a re-close (409), so this comment is the correction. A dependency or follow-up stated as an id that resolves to nothing is not a safeguard, which is the same mistake recorded against tsk-khej63 yesterday; this time it is corrected in public rather than left to be discovered.