Revise PR #283: taosmd/mentions.py is added by BOTH this PR and #233, and here it has no caller

[jaylfc]

CARD TITLE (intent, not commit subject): Revise PR #283: taosmd/mentions.py is added by BOTH this PR and #233, and here it has no caller

Autonomous build of board card tsk-fjy3o7.

• normalise _normalise_handle in authorize_sender identity comparison
  so @-prefixed and bare handles compare equal case-insensitively
• record @mentions in a2a_send via MentionStore so mentions.py has a caller
• update changelog fragment for both changes

Files:
changelog.d/tsk-fjy3o7-mention-revocation.md | 14 +++++
taosmd/mentions.py | 87 ++++++++++++++++++++++++++++
taosmd/registry_auth.py | 5 +-
taosmd/service.py | 38 ++++++++++++
tests/test_identity_normalise.py | 21 +++++++
5 files changed, 163 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: 7 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: 8bca0185-04da-4464-9b4a-0ba6c4a86d6a

📥 Commits

Reviewing files that changed from the base of the PR and between 82987b9 and 18b4b16.

📒 Files selected for processing (5)

• changelog.d/tsk-fjy3o7-mention-revocation.md
• taosmd/mentions.py
• taosmd/registry_auth.py
• taosmd/service.py
• tests/test_identity_normalise.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]

test

[kilo-code-bot]

WARNING: Connection leak in init() if executescript() raises

self._conn is assigned before executescript() runs. If that call throws (e.g. SQLite error, disk full, permission denied), the live connection is left open with no cleanup path — the exception propagates and close() is never called.

Wrap the assignment and executescript in a try/except that closes the connection on failure, or reorder so the connection is only assigned after executescript() succeeds.

---

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

[kilo-code-bot]

WARNING: mention_store connection leaks if archive.record() raises

mention_store is created and init() is called at lines 471–472, but there is no try/finally wrapping the archive.record() call at line 484. If that call raises (e.g. archive storage full, IOError), the exception propagates out of a2a_send without ever reaching the close() calls at lines 499–500, leaking a SQLite connection on every such failure.

Add a try/finally (or async context manager) around the archive write and mention recording to guarantee mention_store.close() runs on all exit paths.

---

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

[kilo-code-bot]

Code Review Summary

Status: 2 Issues Found | Recommendation: Address before merge

Overview

Severity	Count
WARNING	2

Issue Details (click to expand)

WARNING

File	Line	Issue
taosmd/mentions.py	27	Connection leak in init(): self._conn is assigned before executescript() runs; if executescript throws, the connection is never closed
taosmd/service.py	472	mention_store connection leaks if archive.record() raises: no try/finally wraps the archive write, so mention_store.close() at line 500 is never reached on failure

Files Reviewed (5 files)

• taosmd/mentions.py - 1 issue
• taosmd/service.py - 1 issue
• taosmd/registry_auth.py
• tests/test_identity_normalise.py
• changelog.d/tsk-fjy3o7-mention-revocation.md

Fix these issues in Kilo Cloud

---

_{Reviewed by step-3.7-flash · Input: 246.3K · Output: 27.1K · Cached: 1.2M}

[jaylfc]

BLOCKED. The suite is fully green and two things are broken behind it, one of them a revocation bypass.

Reviewed on a trial merge with current master (82987b9). Suite on that tree:

1405 passed, 12 skipped, 0 failed in 165.39s

That green is the reason this review exists rather than an approval. Both defects below are on the tree that would land, and neither is reachable by any test in the suite.

---

BLOCKER 1: _normalise_handle uses re and service.py never imports it. The mentions feature is dead on arrival.

git show origin/exec/tsk-fjy3o7:taosmd/service.py | grep -c '^import re$'   ->  0

_normalise_handle calls re.sub(...) on the mint_strip=True branch. taosmd/mentions.py is the only consumer and it passes mint_strip=True at all three call sites (lines 56, 58, 71). a2a_send now calls record_mentions on every send with a data_dir, which is the normal server path.

Red proof, run on the trial-merge tree, with the control in the same pass:

BOUND TO: .../wt304m/taosmd/service.py
  CONTROL (no @mention):        OK id=1
  REAL PATH (@mention in body): *** NameError: name 're' is not defined

So a plain body works and a body containing an @mention raises. The feature fails on precisely the input it exists to handle.

Why 1405 tests missed it. tests/test_identity_normalise.py has four tests and every one calls _normalise_handle with the default mint_strip=False — the single branch that never touches re:

assert _normalise_handle("@taos-agent") == "taos-agent"
assert _normalise_handle("@TaOS-Agent") == "taos-agent"
assert _normalise_handle("taos-agent") == "taos-agent"
assert _normalise_handle("taos-agent") != _normalise_handle("other-agent")

No test anywhere sends a body with an @mention through a2a_send. The fix is one import line; the missing test is the part that matters, and it must exercise mint_strip=True through a real caller, not through the helper.

---

BLOCKER 2: the revoked check now normalises the key but not the set, so a revoked principal is silently un-revoked.

-    if sub in revoked:
+    if _normalise_handle(sub) in revoked:

parse_revoked_response (registry_auth.py:212) adds the registry feed's canonical_ids to that set raw — it does not normalise them. Normalising only the lookup key means any published id that is not already lowercase-and-bare stops matching.

Red proof, identical script run against both trees, binding printed so neither result is ambiguous:

TREE: .../wt304   BOUND TO: .../wt304/taosmd/registry_auth.py
  revoked set = {'taOSmd-Dev'} ; token sub = 'taOSmd-Dev'
  >>> RESULT: ACCEPTED  *** REVOCATION BYPASSED ***
  >>> CONTROL (all-lowercase id): REJECTED (correct)

TREE: master      BOUND TO: /home/jay/Development/taosmd/taosmd/registry_auth.py
  revoked set = {'taOSmd-Dev'} ; token sub = 'taOSmd-Dev'
  >>> RESULT: REJECTED (correct)
  >>> CONTROL (all-lowercase id): REJECTED (correct)

Master rejects, this branch accepts, and the lowercase control is rejected on both — so the probe can see a working revocation and the difference is the change itself, not the harness.

The card asked for normalisation at the identity comparison (sub vs from). It did not ask for it on the revocation lookup, and that line should go back to an exact match unless the feed's contents are normalised at parse time too. Fix one side only and revocation quietly stops working for any id the registry publishes with a capital letter.

(Full disclosure on method: my first run of this probe lived in /tmp, so sys.path[0] was /tmp and the editable install resolved taosmd to the main checkout. It printed "REJECTED (correct)" for the branch — a false clean. The numbers above are from re-running inside each tree with registry_auth.__file__ printed. Worth knowing for anyone probing a worktree here.)

---

What is good, and must be preserved

The card's actual ask is met: mentions.py has a real caller now, wired into a2a_send. The _normalise_handle docstring is the best one in this family — it says plainly that this is a slug match, not an identity check, and warns it must not be used where same-stem identities have to stay distinct. Keep that wording; it is the right caveat and the other copy of this function does not have it. Do not rebuild from master — the revision card branches from exec/tsk-fjy3o7, which is preserved.

Cross-PR issue, carded separately as tsk-lomnt3

#303 and #304 each add a different _normalise_handle to taosmd/service.py, because tsk-pgtl4b (the card that was supposed to land one shared helper) is closed and its work never reached master. Measured: the two merge in either order with exit 0 and no conflict, leaving grep -c 'def _normalise_handle' taosmd/service.py = 2, with Python silently keeping whichever lands last. Neither lane could see this from inside its own worktree, and no gate we run looks for a duplicate def.

[jaylfc]

BLOCKED and closed. Revision card tsk-7rzind is filed and branches from this PR's head; the branch is deliberately preserved, not deleted - the card depends on it. Do not rebuild this from master.