Skip to content

feat(sharing): notify requester on channel access-request approval (#951) - #977

Merged
vybe merged 2 commits into
devfrom
fix/951-notify-access-request-approval
Jun 1, 2026
Merged

feat(sharing): notify requester on channel access-request approval (#951)#977
vybe merged 2 commits into
devfrom
fix/951-notify-access-request-approval

Conversation

@dolho

@dolho dolho commented May 29, 2026

Copy link
Copy Markdown
Contributor

Summary

After #311 introduced unified channel access control, a user who DMs an agent on Telegram/Slack/WhatsApp without prior sharing is told "I'll let you know once the agent owner responds" — but the decide endpoint never actually told them. Requesters were left guessing whether they'd been admitted.

This wires up the existing proactive-message primitive (#321) to close the loop.

Backend

  • ProactiveMessageService.send_access_grant_notification(agent, email, channel, text) — one-shot path that calls _deliver_via_channel directly and audits the outcome. Bypasses the allow_proactive opt-in (the user explicitly initiated the request; this is the response) and the per-recipient rate limit (one-shot, not a campaign). Failures and missing bindings are caught, audit-logged, and returned as DeliveryResult(success=False, ...) — never raised.
  • routers/sharing.py decide endpoint: when decision.approve AND existing.channel in {telegram, slack, whatsapp}, fires the notification as asyncio.create_task so a missing binding or transport hiccup can't block the HTTP response. Rejections stay silent (deliberate — owners may not want to confirm the agent exists). Web channel handled by the existing agent_shared WebSocket dashboard event.

Notification text mirrors the tone of the original pending-approval reply:

✅ Access to {agent_name} approved by the agent owner. You can now message the agent here.

Docs

  • architecture.md decide-endpoint row + Access Control Flow note call out the post-approval notification and where the audit lands.
  • feature-flows/unified-channel-access-control.md documents the approve flow's new step plus a manual test path for each of Telegram, Slack, WhatsApp, web (negative), and missing-binding (negative).

Tests

4 new unit cases in tests/test_access_grant_notification_unit.py:

  • delivery success → audit success
  • RecipientNotFoundError → audited as recipient_not_found, returned not raised
  • unexpected exception → audited + swallowed (approval must succeed)
  • opt-in allow_proactive bypass verified

Related to #951

Test plan

  • .venv/bin/pytest tests/test_access_grant_notification_unit.py tests/test_proactive_audit_unit.py tests/test_sharing_null_email_unit.py — 13 passed.
  • Live: missing-binding path. Inserted access_request with channel='telegram' for an agent without any Telegram binding, approved via POST /api/agents/.../access-requests/.../decide:
    • Approval HTTP response: 200 (not blocked by the missing transport)
    • GET /api/audit-log?event_type=proactive_message shows new entry with details.channel='telegram', details.success=false, details.error="recipient_not_found: No Telegram bot configured for agent 'trinity-system'", details.message_preview="✅ Access to trinity-system approved by the agent owner. You can now message the agent here."
  • Manual Telegram path: bind a bot, request access from a second account, approve, confirm the approval message lands in the DM. (Documented in feature-flow doc; requires a live Telegram bot to run.)
  • Manual Slack path: same flow against a connected workspace.
  • Manual WhatsApp path: same flow against a Twilio sender.

🤖 Generated with Claude Code

dolho and others added 2 commits May 29, 2026 16:25
)

After #311 introduced the unified channel access-control flow, a user
who DMs an agent on Telegram/Slack/WhatsApp without prior sharing is
told "I'll let you know once the agent owner responds" — but the
decide endpoint never actually told them. Result: requesters were left
guessing whether they'd been admitted.

Wire up the proactive-message primitive (#321) to close the loop.

Backend:
- `ProactiveMessageService.send_access_grant_notification(agent, email,
  channel, text)` — one-shot path that calls `_deliver_via_channel`
  directly and audits the outcome. Bypasses the `allow_proactive`
  opt-in (the user explicitly initiated the request; this is the
  response) and the per-recipient rate limit (one-shot, not a
  campaign). Failures and missing bindings are caught, audit-logged,
  and returned as `DeliveryResult(success=False, ...)` — never raised.
- `routers/sharing.py` decide endpoint: when `decision.approve` AND
  `existing.channel in {telegram, slack, whatsapp}`, fires the
  notification as `asyncio.create_task` so a missing binding or
  transport hiccup can't block the HTTP response. Rejections stay
  silent (deliberate — owners may not want to confirm the agent
  exists). Web channel handled by the existing `agent_shared`
  WebSocket dashboard event.

Notification text mirrors the tone of the original pending-approval
reply so the user recognises it as the promised follow-up:
"✅ Access to {agent_name} approved by the agent owner. You can now
message the agent here."

Docs:
- `architecture.md` decide-endpoint row + Access Control Flow note
  call out the post-approval notification and where the audit lands.
- `feature-flows/unified-channel-access-control.md` documents the
  approve flow's new step plus a manual test path for each of
  Telegram, Slack, WhatsApp, web (negative), and missing-binding
  (negative).

Tests:
- 4 new unit cases in `tests/test_access_grant_notification_unit.py`:
  delivery success → audit success; `RecipientNotFoundError` →
  audited as `recipient_not_found`, returned not raised;
  unexpected exception → audited + swallowed; opt-in bypass verified.

Verified live: approving a telegram-channel request on an agent with
no Telegram binding returns HTTP 200, the approval succeeds, and
`/api/audit-log?event_type=proactive_message` shows
`success=false / error=recipient_not_found / channel=telegram /
message_preview="✅ Access to trinity-system approved..."`.

Related to #951

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The test was using `del sys.modules[mod]` to force a fresh import of
proactive_message_service after stubbing its deps. CI's
`tests/lint_sys_modules.py` baseline gate rejects bare deletions in
new test files — the codebase convention is the snapshot/restore
autouse fixture pattern from
`tests/unit/test_telegram_webhook_backfill.py`.

Switched to `_STUBBED_MODULE_NAMES` + `_restore_sys_modules` autouse
fixture, then `monkeypatch.delitem(..., raising=False)` for the
forced reimport. Same behaviour, no global leakage into sibling
tests, lint baseline stays at 237.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@dolho
dolho requested a review from vybe May 29, 2026 14:13

@vybe vybe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — verified send_access_grant_notification signatures match _deliver_via_channel and _audit_send. Auth boundary checked: decide endpoint is owner-only (OwnedAgentByName), recipient email+channel sourced from the stored access_requests row (not request body — no redirect vector), and the opt-in/rate-limit bypass is justified (one-shot response to a user-initiated, owner-approved request). Fire-and-forget wiring keeps a missing binding from blocking approval; all failure paths audited not raised. Docs updated on both surfaces. CI green, 4 new tests. Validated via /validate-pr.

@vybe
vybe merged commit 321e9fa into dev Jun 1, 2026
20 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants