tsk-b5fz5t [OPEN] A2A: recipient field + role resolution at delivery - #228
Conversation
📝 WalkthroughWalkthroughThis change adds optional A2A recipient handles, role resolution, recipient filtering, delivery-time holder annotations, resolver configuration, remote-client forwarding, and coverage for service, HTTP, SSE, remote, and configuration paths. ChangesA2A role-recipient addressing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant A2AClient
participant HTTPServer
participant RoleResolver
participant A2AService
A2AClient->>HTTPServer: POST /a2a/send with recipient
HTTPServer->>RoleResolver: resolve role recipient
RoleResolver-->>HTTPServer: current holder or resolution error
HTTPServer->>A2AService: a2a_send with verbatim recipient
A2AService-->>HTTPServer: send receipt
HTTPServer-->>A2AClient: send response
A2AClient->>HTTPServer: GET /a2a/messages with recipient filter
HTTPServer->>RoleResolver: resolve role recipient at read time
HTTPServer->>A2AService: a2a_feed with recipient filter
A2AService-->>HTTPServer: stored messages
HTTPServer-->>A2AClient: messages with optional resolved_to
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 QodoA2A: add recipient field and delivery-time @taOS-* role resolution
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
13 rules 1. Role resolve URL wrong
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| except RoleResolveError as exc: | ||
| # Configured but unreachable: 503, never a silent drop. | ||
| self._send_json(503, {"error": f"role resolver unavailable: {exc}"}) | ||
| return |
There was a problem hiding this comment.
1. /a2a/send resolver errors unarchived 📘 Rule violation ☼ Reliability
The new role-recipient validation path returns a 503 on RoleResolveError before calling service.a2a_send(), so no archive.record() occurs for that failed interaction. This violates the requirement to archive every interaction (including failures) via a centralized logger call.
Agent Prompt
## Issue description
`POST /a2a/send` can fail during role-recipient validation (resolver unreachable) and return before any `archive.record()` call happens, so the interaction is not archived.
## Issue Context
Compliance requires that every public interaction is archived regardless of success/failure paths via a centralized archive logger call.
## Fix Focus Areas
- taosmd/http_server.py[1474-1494]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| bare = role.lstrip("@") | ||
| url = self._base + _RESOLVE_PATH.format(role=bare) | ||
| headers: dict[str, str] = {"Accept": "application/json"} |
There was a problem hiding this comment.
2. Role resolve url wrong 🐞 Bug ⛨ Security
RoleResolver._fetch() strips only the leading "@" (leaving the "taOS-" prefix) and interpolates the role into the URL without encoding, so a handle like "@taOS-PA" will call "/api/roles/taOS-PA/resolve" and crafted role strings can alter the request path/query. This can break all role-based sends/reads (unexpected 404/503) and also creates request-target manipulation risk against the configured resolver endpoint.
Agent Prompt
## Issue description
`RoleResolver._fetch()` currently derives the `{role}` path segment via `role.lstrip("@")`, which turns `@taOS-PA` into `taOS-PA` (not the documented bare role name `PA`) and then string-interpolates it into the URL without URL-encoding.
This likely produces the wrong endpoint path and allows a crafted role handle containing reserved characters (e.g. `?`, `#`, `/`) to change the request target.
## Issue Context
Role handles are documented as `@taOS-<name>` and `_RESOLVE_PATH` is `/api/roles/{role}/resolve`, where `{role}` should be the bare `<name>` and must be treated as a single URL path segment.
## Fix Focus Areas
- taosmd/role_resolver.py[34-43]
- taosmd/role_resolver.py[83-126]
### Suggested implementation direction
- Derive the name as `name = role[len(ROLE_PREFIX):]` (after confirming `role.startswith(ROLE_PREFIX)`), not via `lstrip("@")`.
- Validate `name` (e.g., allow `[A-Za-z0-9_-]+` only) and reject/return `None` otherwise.
- URL-encode the segment with `urllib.parse.quote(name, safe="")` before formatting into `_RESOLVE_PATH`.
- Consider using `urllib.parse.urljoin` or equivalent to avoid accidental `//` behavior when composing base + path.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if recipient is not None: | ||
| if not isinstance(recipient, str) or not recipient.strip(): | ||
| raise _BadRequest("'recipient' must be a non-empty string when provided") | ||
| recipient = recipient.strip() |
There was a problem hiding this comment.
3. Recipient size unchecked 🐞 Bug ☼ Reliability
POST /a2a/send now accepts and persists recipient, but the existing 64KB size guard only covers
{body, refs, blocks} and the new recipient validation enforces only “non-empty”. A client can
bypass the envelope size guard with an arbitrarily large recipient, causing oversized archive rows
and large echoed responses.
Agent Prompt
## Issue description
The A2A send handler enforces a 64KB limit by serializing only `body/refs/blocks`, but it now also accepts `recipient` and stores/returns it. Because `recipient` is not included in the size computation and has no explicit maximum length, requests can store/echo very large `recipient` values.
## Issue Context
The handler already aims to bound envelope size to prevent oversized messages. The new field should obey the same bound (or have its own strict limit), since it is persisted and returned in receipts/feeds.
## Fix Focus Areas
- taosmd/http_server.py[1415-1455]
- taosmd/http_server.py[1468-1495]
- taosmd/service.py[405-426]
### Suggested implementation direction
- Add a max length for `recipient` (e.g. 256/1024) and enforce it in `_handle_a2a_send`.
- Also consider adding `recipient` into the serialized size check (or perform a second size check over the full envelope including `thread/reply_to/recipient`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if recipient is not None and ( | ||
| not isinstance(recipient, str) or not recipient | ||
| ): | ||
| raise ValueError("recipient must be a non-empty string when provided") |
There was a problem hiding this comment.
4. Whitespace recipient allowed 🐞 Bug ≡ Correctness
service.a2a_send() validates recipient using not recipient rather than `not recipient.strip()`, so direct (non-HTTP) callers can store whitespace-only recipients. HTTP strips recipient values on write and on read filtering, so those stored whitespace recipients become difficult/impossible to retrieve via GET /a2a/messages?recipient=.
Agent Prompt
## Issue description
The service layer allows whitespace-only `recipient` values because it checks `not recipient` instead of `not recipient.strip()`. This creates inconsistent behavior vs the HTTP layer (which strips and rejects whitespace-only recipients).
## Issue Context
While the HTTP server normalizes recipients, other in-process callers may use `service.a2a_send()` directly (tests/tools), and should get consistent validation/normalization.
## Fix Focus Areas
- taosmd/service.py[383-412]
- taosmd/http_server.py[1474-1478]
- taosmd/http_server.py[1586-1590]
### Suggested implementation direction
- In `service.a2a_send()`, if `recipient is not None`, require `isinstance(recipient, str) and recipient.strip()`.
- Decide on a single normalization rule (store stripped value vs store verbatim) and apply it consistently across HTTP + service.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
tests/test_a2a.py (4)
891-893: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the unused
resolverbinding.Ruff reports RUF059:
resolveris never used in this test. The test relies on the default empty holder map.♻️ Proposed change
def test_role_send_unresolvable_returns_400(resolver_server): """An unresolvable role (no holder) -> 400 naming the role, nothing stored.""" - url, resolver = resolver_server + url, _resolver = resolver_server🤖 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.py` around lines 891 - 893, Remove the unused resolver binding from test_role_send_unresolvable_returns_400 while preserving the resolver_server setup and URL usage; keep the test relying on the default empty holder map.Source: Linters/SAST tools
932-932: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the test to match what it asserts.
The name says
verbosely_stored_even_when_role_value_is_arbitrary. The test stores a non-role handle verbatim and asserts that no resolver is consulted. "verbosely" reads as a typo for "verbatim", and the name mentions a role value that the test does not use.📝 Proposed rename
-def test_role_send_verbosely_stored_even_when_role_value_is_arbitrary(live_server): +def test_non_role_recipient_stored_verbatim_without_resolver(live_server):🤖 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.py` at line 932, Rename the test function test_role_send_verbosely_stored_even_when_role_value_is_arbitrary to accurately describe that a non-role handle is stored verbatim without consulting a resolver; remove the misleading “verbosely” and role-value wording while preserving the test behavior.
732-760: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
resolver_serverduplicateslive_serverexcept for one argument.Both fixtures create the data dir, reset
_stores_cache, callmake_server, patch the embedder, start the thread, and run the identical teardown block. Only therole_resolverargument differs. Extract the common body into one helper so a teardown fix has to be made once.♻️ Proposed refactor sketch
+@contextlib.contextmanager +def _serve(tmp_path, monkeypatch, name, **make_server_kwargs): + data_dir = tmp_path / name + data_dir.mkdir() + monkeypatch.setattr(taosmd_api, "_stores_cache", {}) + httpd = http_server.make_server("127.0.0.1", 0, data_dir=str(data_dir), **make_server_kwargs) + stores = httpd.service_loop.run(taosmd_api._ensure_stores(str(data_dir))) + _patch_embedder(stores) + host, port = httpd.server_address[:2] + t = threading.Thread(target=httpd.serve_forever, daemon=True) + t.start() + try: + yield f"http://{host}:{port}" + finally: + httpd.shutdown() + httpd.server_close() + t.join(timeout=5) + for s in list(taosmd_api._stores_cache.values()): + for store in (s.get("archive"), s.get("vector"), s.get("kg")): + if store and hasattr(store, "close"): + try: + httpd.service_loop.run(store.close()) + except Exception: + logger.debug("store close failed during teardown", exc_info=True) + httpd.service_loop.close() + + `@pytest.fixture` def resolver_server(tmp_path, monkeypatch): """HTTP server with an injected fake role resolver; yields (url, resolver).""" - data_dir = tmp_path / "taosmd-a2a-resolver" - ... + resolver = _FakeRoleResolver() + with _serve(tmp_path, monkeypatch, "taosmd-a2a-resolver", role_resolver=resolver) as url: + yield url, resolverThe helper also removes the bare
try/except/passthat Ruff reports at lines 758-759 (S110, 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.py` around lines 732 - 760, Extract the shared server setup, thread startup, yield, and teardown from resolver_server and live_server into a helper fixture or function that accepts the optional role_resolver argument. Update both fixtures to use this helper while preserving their existing yielded values and behavior, and replace the teardown’s broad bare exception suppression with targeted handling that satisfies Ruff’s S110 and BLE001 checks.Source: Linters/SAST tools
708-729: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct unit tests for
RoleResolver.Every role test drives
_FakeRoleResolver. The realtaosmd/role_resolver.pytransport and cache logic stays untested: the 404-to-Nonemapping, the non-404 HTTP error toRoleResolveErrormapping, the TTL cache, andbust(). A regression in those paths would not fail this suite.Do you want me to generate unit tests for
RoleResolveragainst a local stub HTTP server, or open an issue to track this?🤖 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.py` around lines 708 - 729, Add direct unit tests for taosmd.role_resolver.RoleResolver using a local stub HTTP server or equivalent transport stub. Cover 404 responses returning None, non-404 HTTP failures raising RoleResolveError, TTL cache behavior, and bust() forcing a fresh lookup; keep _FakeRoleResolver-based integration tests unchanged.taosmd/role_resolver.py (1)
139-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSort
__all__to satisfy Ruff RUF022.Ruff reports
__all__is not sorted. Apply isort-style ordering.♻️ Proposed change
__all__ = [ + "ROLE_PREFIX", + "RoleResolveError", "RoleResolver", - "RoleResolveError", "is_role_handle", - "ROLE_PREFIX", ]🤖 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/role_resolver.py` around lines 139 - 144, Sort the entries in the module-level __all__ declaration in isort/Ruff RUF022 order, while retaining all existing exports: ROLE_PREFIX, RoleResolveError, RoleResolver, and is_role_handle.Source: Linters/SAST tools
taosmd/http_server.py (1)
1584-1608: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
_BadRequestfor the missing-resolver case, as the send path does.Line 1593 writes the 400 with
self._send_jsonand returns._handle_a2a_sendraises_BadRequestfor the same condition, and_dispatchconverts it to a 400. Use the same mechanism here so the two paths stay aligned and the line stays readable.♻️ Proposed change
if is_role_handle(recipient): annotate = True if _role_resolver is None: - self._send_json(400, {"error": f"role recipient {recipient!r} requires a configured role resolver (a2a_role_resolver_url)"}) - return + raise _BadRequest( + f"role recipient {recipient!r} requires a configured " + "role resolver (a2a_role_resolver_url)" + )🤖 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 1584 - 1608, Update the missing-resolver branch in the role-recipient handling to raise _BadRequest with the appropriate message instead of calling self._send_json and returning. Keep the existing RoleResolveError 503 handling unchanged, so _dispatch can convert the bad-request exception to a 400 consistently with _handle_a2a_send.
🤖 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 1468-1494: Reorder recipient handling in the request flow so only
shape validation for a provided recipient runs before the registry
authentication and grant checks. Move the is_role_handle resolution logic,
including resolver configuration, error, and holder validation, to immediately
after the registry auth block and before the service.a2a_send call, preserving
the existing responses and stored verbatim recipient behavior.
In `@taosmd/remote.py`:
- Around line 243-249: Update the GET /a2a/messages docstring in RemoteClient to
accurately state that the remote server may annotate role-recipient messages
with resolved_to and that RemoteClient returns this annotation unchanged; remove
the contradictory claim that callers perform delivery-time resolution.
In `@taosmd/role_resolver.py`:
- Around line 89-102: Update role validation in resolve and _fetch so role names
after the `@taOS-` prefix accept only a strict safe character set, reject invalid
handles before cache lookup or URL construction, and percent-encode the
validated name when formatting _RESOLVE_PATH. Bound self._cache with an eviction
policy or maximum size while preserving existing TTL and negative-result
behavior.
- Around line 19-21: Update the module docstring near resolve() to state that
role resolution is cached for _ttl seconds, including send-time lookups, and
that holder rotation becomes visible after the TTL expires rather than on the
next read.
In `@taosmd/service.py`:
- Around line 526-529: The recipient filter in the archive query path must not
be limited by the initial page size. When recipient is set, over-fetch results
before applying the filter, then trim the filtered result to limit before
returning; follow the existing alias-merge over-fetch behavior and preserve
current handling when no recipient filter is provided.
---
Nitpick comments:
In `@taosmd/http_server.py`:
- Around line 1584-1608: Update the missing-resolver branch in the
role-recipient handling to raise _BadRequest with the appropriate message
instead of calling self._send_json and returning. Keep the existing
RoleResolveError 503 handling unchanged, so _dispatch can convert the
bad-request exception to a 400 consistently with _handle_a2a_send.
In `@taosmd/role_resolver.py`:
- Around line 139-144: Sort the entries in the module-level __all__ declaration
in isort/Ruff RUF022 order, while retaining all existing exports: ROLE_PREFIX,
RoleResolveError, RoleResolver, and is_role_handle.
In `@tests/test_a2a.py`:
- Around line 891-893: Remove the unused resolver binding from
test_role_send_unresolvable_returns_400 while preserving the resolver_server
setup and URL usage; keep the test relying on the default empty holder map.
- Line 932: Rename the test function
test_role_send_verbosely_stored_even_when_role_value_is_arbitrary to accurately
describe that a non-role handle is stored verbatim without consulting a
resolver; remove the misleading “verbosely” and role-value wording while
preserving the test behavior.
- Around line 732-760: Extract the shared server setup, thread startup, yield,
and teardown from resolver_server and live_server into a helper fixture or
function that accepts the optional role_resolver argument. Update both fixtures
to use this helper while preserving their existing yielded values and behavior,
and replace the teardown’s broad bare exception suppression with targeted
handling that satisfies Ruff’s S110 and BLE001 checks.
- Around line 708-729: Add direct unit tests for
taosmd.role_resolver.RoleResolver using a local stub HTTP server or equivalent
transport stub. Cover 404 responses returning None, non-404 HTTP failures
raising RoleResolveError, TTL cache behavior, and bust() forcing a fresh lookup;
keep _FakeRoleResolver-based integration tests unchanged.
🪄 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: 97cb3c02-2b31-4e6a-96d0-dc9540e775e8
📒 Files selected for processing (7)
taosmd/config.pytaosmd/http_server.pytaosmd/remote.pytaosmd/role_resolver.pytaosmd/service.pytests/test_a2a.pytests/test_config_role_resolver.py
| # --- Recipient validation (taOSmd #2155) --- | ||
| # recipient: optional addressee handle (agent @handle or role | ||
| # @taOS-<name>). Stored VERBATIM. A role recipient is validated at | ||
| # send time against the configured resolver -- the binding happens at | ||
| # delivery, so the resolved identity is never persisted and holder | ||
| # rotation needs no sender reconfiguration. | ||
| if recipient is not None: | ||
| if not isinstance(recipient, str) or not recipient.strip(): | ||
| raise _BadRequest("'recipient' must be a non-empty string when provided") | ||
| recipient = recipient.strip() | ||
| if is_role_handle(recipient): | ||
| if _role_resolver is None: | ||
| raise _BadRequest( | ||
| f"role recipient {recipient!r} requires a configured " | ||
| "role resolver (a2a_role_resolver_url)" | ||
| ) | ||
| try: | ||
| holder = _role_resolver.resolve(recipient) | ||
| except RoleResolveError as exc: | ||
| # Configured but unreachable: 503, never a silent drop. | ||
| self._send_json(503, {"error": f"role resolver unavailable: {exc}"}) | ||
| return | ||
| # A None result means no single holder -> reject, nothing stored. | ||
| if holder is None: | ||
| raise _BadRequest( | ||
| f"role recipient {recipient!r} does not resolve to a single holder" | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Run the registry auth checks before the role resolution.
This block resolves the role at line 1485. The registry identity and grant checks start at line 1501. An unauthenticated caller therefore reaches the resolver first. Two effects follow when a2a_auth_enforce is on:
- The caller learns whether a role has a current holder. A 400 means no holder, and passing this block means a holder exists. The 401/403 arrives only afterwards.
- Each rejected request still triggers an outbound request to the taOS resolution endpoint on a cache miss, so an unauthenticated caller can drive resolver traffic.
Move the recipient shape validation before the auth block if you want a cheap 400, and move the resolver call after the auth block.
🛡️ Proposed reordering
if recipient is not None:
if not isinstance(recipient, str) or not recipient.strip():
raise _BadRequest("'recipient' must be a non-empty string when provided")
recipient = recipient.strip()
- if is_role_handle(recipient):
- if _role_resolver is None:
- raise _BadRequest(
- f"role recipient {recipient!r} requires a configured "
- "role resolver (a2a_role_resolver_url)"
- )
- try:
- holder = _role_resolver.resolve(recipient)
- except RoleResolveError as exc:
- # Configured but unreachable: 503, never a silent drop.
- self._send_json(503, {"error": f"role resolver unavailable: {exc}"})
- return
- # A None result means no single holder -> reject, nothing stored.
- if holder is None:
- raise _BadRequest(
- f"role recipient {recipient!r} does not resolve to a single holder"
- )
# Registry auth (opt-in): ...Then place the role-resolution block immediately after the registry auth block and before the service.a2a_send call.
🤖 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 1468 - 1494, Reorder recipient handling
in the request flow so only shape validation for a provided recipient runs
before the registry authentication and grant checks. Move the is_role_handle
resolution logic, including resolver configuration, error, and holder
validation, to immediately after the registry auth block and before the
service.a2a_send call, preserving the existing responses and stored verbatim
recipient behavior.
| """GET /a2a/messages: return messages from the remote A2A bus, oldest-first. | ||
|
|
||
| Returns the ``messages`` list from the server response. | ||
| ``recipient`` is forwarded as ``?recipient=`` so the remote server can | ||
| apply its verbatim recipient filter (taOSmd #2155). The remote does | ||
| not annotate ``resolved_to``; callers reading a role recipient do their | ||
| own delivery-time resolution. | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The docstring contradicts the server behavior verified by the tests.
Lines 246-248 state that the remote does not annotate resolved_to and that callers do their own delivery-time resolution. _handle_a2a_messages in taosmd/http_server.py annotates resolved_to for a role recipient, and tests/test_a2a.py asserts msgs[0]["resolved_to"] == "holderA" after rc.a2a_feed(...). RemoteClient returns that annotation unchanged.
📝 Proposed docstring fix
``recipient`` is forwarded as ``?recipient=`` so the remote server can
- apply its verbatim recipient filter (taOSmd `#2155`). The remote does
- not annotate ``resolved_to``; callers reading a role recipient do their
- own delivery-time resolution.
+ apply its verbatim recipient filter (taOSmd `#2155`). For a role
+ recipient the server resolves the current holder at read time and adds
+ ``resolved_to`` to each message; this client returns that field
+ unchanged. The server returns 400 when no resolver is configured and
+ 503 when the resolver is unreachable, which surface as ``RuntimeError``.📝 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.
| """GET /a2a/messages: return messages from the remote A2A bus, oldest-first. | |
| Returns the ``messages`` list from the server response. | |
| ``recipient`` is forwarded as ``?recipient=`` so the remote server can | |
| apply its verbatim recipient filter (taOSmd #2155). The remote does | |
| not annotate ``resolved_to``; callers reading a role recipient do their | |
| own delivery-time resolution. | |
| """ | |
| """GET /a2a/messages: return messages from the remote A2A bus, oldest-first. | |
| ``recipient`` is forwarded as ``?recipient=`` so the remote server can | |
| apply its verbatim recipient filter (taOSmd `#2155`). For a role | |
| recipient the server resolves the current holder at read time and adds | |
| ``resolved_to`` to each message; this client returns that field | |
| unchanged. The server returns 400 when no resolver is configured and | |
| 503 when the resolver is unreachable, which surface as ``RuntimeError``. | |
| """ |
🤖 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 243 - 249, Update the GET /a2a/messages
docstring in RemoteClient to accurately state that the remote server may
annotate role-recipient messages with resolved_to and that RemoteClient returns
this annotation unchanged; remove the contradictory claim that callers perform
delivery-time resolution.
| Resolution is NEVER cached at send time -- the stored envelope keeps only the | ||
| role handle, so rotation is reflected on the next read. A short TTL cache | ||
| bounds repeated lookups within a single resolver instance. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the caching statement in the module docstring.
Line 19 states that resolution is never cached at send time. resolve() caches every answer for _ttl seconds, including at send time. Holder rotation is therefore visible only after the TTL expires, not on the next read. Restate the contract so operators know the staleness window.
📝 Proposed docstring fix
-Resolution is NEVER cached at send time -- the stored envelope keeps only the
-role handle, so rotation is reflected on the next read. A short TTL cache
-bounds repeated lookups within a single resolver instance.
+The resolved identity is NEVER persisted -- the stored envelope keeps only the
+role handle, so rotation needs no re-send. Lookups are answered from a short
+TTL cache (``ttl``, default 30s), so a rotation becomes visible on the first
+read after the cached entry expires. Call :meth:`RoleResolver.bust` to make a
+rotation visible immediately.📝 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.
| Resolution is NEVER cached at send time -- the stored envelope keeps only the | |
| role handle, so rotation is reflected on the next read. A short TTL cache | |
| bounds repeated lookups within a single resolver instance. | |
| The resolved identity is NEVER persisted -- the stored envelope keeps only the | |
| role handle, so rotation needs no re-send. Lookups are answered from a short | |
| TTL cache (``ttl``, default 30s), so a rotation becomes visible on the first | |
| read after the cached entry expires. Call :meth:`RoleResolver.bust` to make a | |
| rotation visible immediately. |
🤖 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/role_resolver.py` around lines 19 - 21, Update the module docstring
near resolve() to state that role resolution is cached for _ttl seconds,
including send-time lookups, and that holder rotation becomes visible after the
TTL expires rather than on the next read.
| if not is_role_handle(role): | ||
| return None | ||
| with self._lock: | ||
| cached = self._cache.get(role) | ||
| if cached is not None and cached[0] > time.monotonic(): | ||
| return cached[1] | ||
| result = self._fetch(role) | ||
| with self._lock: | ||
| self._cache[role] = (time.monotonic() + self._ttl, result) | ||
| return result | ||
|
|
||
| def _fetch(self, role: str) -> str | None: | ||
| bare = role.lstrip("@") | ||
| url = self._base + _RESOLVE_PATH.format(role=bare) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate the role name before you build the URL and the cache key.
role reaches this method straight from the recipient field of an A2A send or feed request. is_role_handle checks only the @taOS- prefix. Two consequences follow:
_fetchinterpolatesbareinto_RESOLVE_PATHwithout percent-encoding. A recipient such as@taOS-a/../../adminor@taOS-a?x=1changes the request path and query sent to the internal taOS endpoint.resolvecaches one entry per distinct role string, including negative answers. A caller can send many unique@taOS-<random>handles and growself._cachewithout bound.
Validate the role name against a strict character set and percent-encode it. Bound the cache as well.
🛡️ Proposed fix
+import re
+import urllib.parse
+
+# A role name is a bounded, conservative identifier; anything else cannot
+# address a taOS role and must never reach the resolver URL.
+_ROLE_NAME_RE = re.compile(r"^[A-Za-z0-9._-]{1,64}$")
+
def resolve(self, role: str) -> str | None:
@@
if not is_role_handle(role):
return None
+ if not _ROLE_NAME_RE.match(role[len(ROLE_PREFIX):]):
+ return None
with self._lock:
@@
def _fetch(self, role: str) -> str | None:
- bare = role.lstrip("@")
- url = self._base + _RESOLVE_PATH.format(role=bare)
+ bare = role.lstrip("@")
+ url = self._base + _RESOLVE_PATH.format(
+ role=urllib.parse.quote(bare, safe="")
+ )🤖 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/role_resolver.py` around lines 89 - 102, Update role validation in
resolve and _fetch so role names after the `@taOS-` prefix accept only a strict
safe character set, reject invalid handles before cache lookup or URL
construction, and percent-encode the validated name when formatting
_RESOLVE_PATH. Bound self._cache with an eviction policy or maximum size while
preserving existing TTL and negative-result behavior.
Source: Linters/SAST tools
| # Verbatim recipient filter: matches the stored handle (agent or role) | ||
| # exactly. Role resolution/annotation is the HTTP layer's job. | ||
| if recipient is not None and data.get("recipient") != recipient: | ||
| continue |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The recipient filter runs after limit, so matching messages can be lost.
archive.query applies limit first. The recipient filter then discards rows from that page. A thread with 50 recent unaddressed messages and older messages addressed to @agent1 returns an empty list for recipient=@agent1`` with the default limit=50. The caller sees no messages even though matching messages exist.
The same page-then-filter pattern already exists for _superseded and _deleted, but those cases are rare. A recipient filter is highly selective, so the effect is common.
Over-fetch when a recipient filter is set, in the same way the alias-merge branch already over-fetches, then trim to limit.
🐛 Proposed fix
# Query with no thread filter when we need to merge history from aliases
+ # A recipient filter is applied per row below, so page-then-filter can drop
+ # every match. Over-fetch when it is set, then trim to ``limit``.
+ fetch_limit = limit * 10 if recipient is not None else limit
if alias_sources and thread is not None:
- rows_all = await archive.query(event_type=EVENT_A2A, since=since, limit=limit * 10)
+ rows_all = await archive.query(event_type=EVENT_A2A, since=since, limit=fetch_limit * 10)
rows = [
r for r in rows_all
if (r.get("app_id") == thread or r.get("app_id") in alias_sources)
]
- rows = rows[:limit]
+ rows = rows[:fetch_limit]
else:
rows = await archive.query(
event_type=EVENT_A2A,
app_id=thread,
since=since,
- limit=limit,
+ limit=fetch_limit,
)Then cap result at limit before returning:
result.append(msg)
- return result
+ return result[-limit:] if recipient is not None else result🤖 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 526 - 529, The recipient filter in the
archive query path must not be limited by the initial page size. When recipient
is set, over-fetch results before applying the filter, then trim the filtered
result to limit before returning; follow the existing alias-merge over-fetch
behavior and preserve current handling when no recipient filter is provided.
|
Deep review done. Verdict FIX - and a big improvement on #225: everything parses, the import fails loud, roles are a clean @taOS- prefix convention (a plain-handle DM with a resolver configured now returns 200, verified live), the 400/503 mapping is real, a2a_feed emits recipient on all three read paths, remote.py has full parity, and 51 tests pass with mutation-proven coverage (breaking role detection fails 8, breaking storage fails 9). Coupling is fine as one PR - service/remote are role-agnostic and roles are inert with no resolver configured, so no split needed. BLOCKING, in priority order:
|
Blocked on the title, plus what I have and have not verifiedBLOCKER — the title misdescribes the change. The PR is titled What I verified and accept:
What I have NOT verified, stated so nobody reads this as a full approval:
Not a rejection. Fix the title and the substantive review follows. |
jaylfc
changed the title
|
Title fixed by me rather than bounced back to the lane, since the lane's run is long over and the correct title was recoverable from the PR's own first commit. Now reads For the record, this was not a lane error and needs no card: My blocker on this PR is cleared. The substantive review of the roles/recipient logic follows. |
CHANGES REQUESTED — the role namespace collides with live agent handlesSubstantive review, as promised once the title was fixed. The implementation quality is good: the fail-loud contract in The blocker is the namespace, not the code.
Addressing the fleet lead by name stops working. With a resolver configured it is no better: Why CI did not catch it: every test uses an invented role name — This is a design decision, not a lane fix, so I am not asking the lane to guess. The role namespace must not overlap the agent-handle namespace. Options: a distinct prefix that cannot collide ( Whichever is chosen, add a test that a |
Namespace decision is in — rework contract on the card@taOS-dev has ruled on the blocker I raised (A2A 2186). Summary, with the full acceptance contract added as a comment on card
The test requirement is the important part: use a real The fail-loud contract, the no-send-time-caching decision and the injected resolver seam are all good and should survive the rework. |
This reverts merge 60c2080, which I pushed to master by mistake: I ran 'git push origin HEAD:master' from a working copy sitting on exec/tsk-nwg6ef, which carried the merge along with the intended docs commit. PR #228 is BLOCKED with changes requested. Its role_resolver.py classifies the live agent handles @taOS-dev and @taOS-website-dev as roles, which 400s messages to them, and @taOS-dev's decision (A2A 2186) requires a @ROLE: namespace plus a static allowlist before it lands. Master must not carry it until that reworks. Not deployed: the Pi runs d0392a7 and a live probe with recipient=@taOS-dev returned 200, so production was never affected. The work is untouched on exec/tsk-b5fz5t and in PR #228; this only removes it from master.
1 participant
Autonomous build of board card tsk-b5fz5t.
Files:
taosmd/config.py | 53 +++++
taosmd/http_server.py | 111 +++++++++--
taosmd/remote.py | 13 +-
taosmd/role_resolver.py | 144 ++++++++++++++
taosmd/service.py | 45 ++++-
tests/test_a2a.py | 385 +++++++++++++++++++++++++++++++++++++
tests/test_config_role_resolver.py | 44 +++++
7 files changed, 777 insertions(+), 18 deletions(-)
Summary by CodeRabbit
New Features
@taOS-role handles.Bug Fixes