tsk-gp5xrq [OPEN] S2A: GET /api/share/destinations (authorization-fi - #2146
Conversation
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 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 device-auth share destinations endpoint (/api/share/destinations)
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
|
nemotron-ultra-orB review Now let me do the thorough code review of the diff:
Automated first-pass review by the nemotron-ultra-orB lane. The lead still reviews before merge. |
|
nemotron-super review VERDICT: Blocking issues found
Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
|
nemotron-ultra-kilo review VERDICT: Needs fixes - security bypass, logic errors, and test gaps
Automated first-pass review by the nemotron-ultra-kilo lane. The lead still reviews before merge. |
Code Review by Qodo
1. share_router missing _csrf dependencies
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| from tinyagentos.routes.share import router as share_router | ||
| app.include_router(share_router) |
There was a problem hiding this comment.
1. share_router missing _csrf dependencies 📜 Skill insight ⌂ Architecture
tinyagentos.routes.share is included without dependencies=_csrf in register_all_routers(), violating the required router registration pattern. This can bypass the standard CSRF dependency wiring expected across routes.
Agent Prompt
## Issue description
`tinyagentos.routes.share` is registered without `dependencies=_csrf` in `register_all_routers()`, which violates the router registration requirement.
## Issue Context
Other routers in `register_all_routers()` are consistently included with `dependencies=_csrf`.
## Fix Focus Areas
- tinyagentos/routes/__init__.py[65-66]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| import pytest | ||
| from httpx import ASGITransport, AsyncClient | ||
|
|
||
| from tinyagentos.app import create_app | ||
| from tinyagentos.device_store import DeviceStore | ||
| from tinyagentos.agent_registry_store import AgentRegistryStore |
There was a problem hiding this comment.
3. Test filename doesn't mirror module 📜 Skill insight ⚙ Maintainability
A new route module tinyagentos/routes/share.py was added, but the new test file is named tests/test_routes_share_destinations.py instead of mirroring the module name (tests/test_share.py). This violates the test naming/mirroring convention and makes the suite harder to navigate.
Agent Prompt
## Issue description
The newly added route module `tinyagentos/routes/share.py` does not have a correspondingly named test file `tests/test_share.py`.
## Issue Context
Compliance requires test files to mirror the module structure for new route modules.
## Fix Focus Areas
- tests/test_routes_share_destinations.py[1-117]
- tinyagentos/routes/share.py[1-63]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| for member in members: | ||
| if member in (user_id_str, "user"): | ||
| continue | ||
| if member in seen: | ||
| continue | ||
| label = member | ||
| if registry is not None: | ||
| try: | ||
| agent = await registry.get(member) | ||
| if agent and agent.get("status") == "active": | ||
| seen.add(member) | ||
| destinations.append({ | ||
| "kind": "agent_chat", | ||
| "id": member, | ||
| "label": agent.get("display_name") or member, | ||
| }) | ||
| except RuntimeError: | ||
| pass |
There was a problem hiding this comment.
4. Agent chat id mismatch 🐞 Bug ≡ Correctness
list_share_destinations only emits "agent_chat" destinations when registry.get(member) returns an active registry record, but local agent DM channels are created with the agent slug (body.name) as a channel member, not a registry canonical_id. This makes /api/share/destinations omit those agent chat targets because AgentRegistryStore.get() queries by canonical_id and returns None for slugs.
Agent Prompt
## Issue description
`GET /api/share/destinations` currently determines agent chat destinations by calling `registry.get(member)` for each non-user member in relevant chat channels. That lookup is by registry `canonical_id`, but existing DM channels for locally deployed agents are created with `members=["user", body.name]` where `body.name` is the agent slug. As a result, locally deployed agent chats will not show up in the destinations list.
## Issue Context
- `tinyagentos/routes/share.py` treats each channel member string as a registry key and requires `agent['status'] == 'active'` to add an `agent_chat` destination.
- `tinyagentos/routes/agents.py` creates DM channels with the agent slug as the member.
- `AgentRegistryStore.get(canonical_id)` only resolves by `canonical_id`.
## Fix Focus Areas
- Add fallback resolution for non-canonical member IDs:
- If `registry.get(member)` returns None, try resolving `member` as a locally configured agent slug (e.g., via config lookup) and include it as an `agent_chat` destination when appropriate.
- Alternatively, change DM-channel creation to store the registry `canonical_id` when the agent has/gets a registry identity, and keep slug compatibility during a migration window.
- Add a test that creates a DM channel with `members=["user", <slug>]` and asserts it appears in destinations.
### Fix Focus Areas (code references)
- tinyagentos/routes/share.py[36-61]
- tinyagentos/routes/agents.py[688-705]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| ch_store = request.app.state.chat_channels | ||
| seen = set() | ||
| registry = getattr(request.app.state, "agent_registry", None) | ||
| user_id_str = str(user_id) | ||
| for ch in await ch_store.list_channels(): | ||
| members = ch.get("members") or [] | ||
| if user_id_str not in members and "user" not in members: | ||
| continue |
There was a problem hiding this comment.
5. Scans all chat channels 🐞 Bug ➹ Performance
list_share_destinations calls ch_store.list_channels() with no filters and then performs Python-side membership checks, causing work proportional to total channels and members plus serial registry lookups. ChatChannelStore.list_channels supports a member_id SQL filter, so the endpoint can be scoped to the requesting user (and optionally a second query for legacy "user") to avoid this O(total_channels) behavior.
Agent Prompt
## Issue description
`GET /api/share/destinations` currently loads *all* chat channels (`await ch_store.list_channels()`) and filters them in Python. On instances with many channels this becomes an avoidable latency and DB load hotspot, especially since the loop may also trigger many `registry.get()` calls.
## Issue Context
`ChatChannelStore.list_channels()` already supports `member_id=...` filtering at the SQL layer (via a `members LIKE ?` clause). This endpoint can call it with the requester's member id instead of reading the full table.
## Fix Focus Areas
- Replace `await ch_store.list_channels()` with a member-filtered query:
- `channels = await ch_store.list_channels(member_id=user_id_str, archived=False)`
- If legacy channels use the literal `"user"` member, merge results from a second query `member_id="user"` only when needed.
- Keep the existing in-Python membership check as a defensive filter if desired, but ensure the DB query is already scoped.
### Fix Focus Areas (code references)
- tinyagentos/routes/share.py[36-43]
- tinyagentos/chat/channel_store.py[127-156]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Reviewed, including verifying both bot findings against the code rather than taking them at face value. Good PR - real tests (401 unauthenticated, cross-user project filtering, response shape), and the design is right. The EXEMPT_PATHS addition is NOT a security hole, to head off that reading: the route calls Three things before it merges, all fix-forward on this branch: 1. doc-gate is red, correctly. A new API route module was added, so the gate wants 2. Missing CSRF dependency (Qodo #1) - confirmed real. 3. Agent chat destinations are silently omitted (Qodo #4) - confirmed real. Also note: this conflicts with #2145, which touches the same |
|
Merging. I checked the auth-exempt change carefully because that is exactly where a public-data hole hides, and it is sound. Adding Clean, gated, scoped, tested. Good. |
1 participant
Autonomous build of board card tsk-gp5xrq.
Files:
tests/test_routes_share_destinations.py | 117 ++++++++++++++++++++++++++++++++
tinyagentos/auth_middleware.py | 2 +-
tinyagentos/routes/init.py | 3 +
tinyagentos/routes/share.py | 63 +++++++++++++++++
4 files changed, 184 insertions(+), 1 deletion(-)