-
Notifications
You must be signed in to change notification settings - Fork 0
feat(cron/slack): flat in-channel continuable cron delivery surface #260
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -427,6 +427,14 @@ class SlackAdapter(BasePlatformAdapter): | |||||||||||||||||||||||||||||||||
| # the prefix that works everywhere — instruction text must show it. | ||||||||||||||||||||||||||||||||||
| typed_command_prefix = "!" | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| # Slack has both halves the ``in_channel`` continuable-cron surface needs: | ||||||||||||||||||||||||||||||||||
| # a flat-reply outbound gate (``reply_in_thread: false`` → ``_resolve_thread_ts`` | ||||||||||||||||||||||||||||||||||
| # returns None for top-level channel messages) AND a whole-channel inbound | ||||||||||||||||||||||||||||||||||
| # session bucket keyed ``(platform, channel_id, None)`` (the same | ||||||||||||||||||||||||||||||||||
| # ``reply_in_thread: false`` path in ``_handle_slack_message``). So a | ||||||||||||||||||||||||||||||||||
| # continuable cron delivered flat here continues in-context on a plain reply. | ||||||||||||||||||||||||||||||||||
| supports_inchannel_continuable = True | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| def __init__(self, config: PlatformConfig): | ||||||||||||||||||||||||||||||||||
| super().__init__(config, Platform.SLACK) | ||||||||||||||||||||||||||||||||||
| self._app: Optional[Any] = None | ||||||||||||||||||||||||||||||||||
|
|
@@ -1073,6 +1081,7 @@ async def connect(self, *, is_reconnect: bool = False) -> bool: | |||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| self._warn_if_missing_group_dm_scopes(auth_response, team_name) | ||||||||||||||||||||||||||||||||||
| self._warn_if_not_bot_token(auth_response, team_name) | ||||||||||||||||||||||||||||||||||
| self._warn_if_inchannel_without_flat_reply(team_name) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| # Register message event handler | ||||||||||||||||||||||||||||||||||
| @self._app.event("message") | ||||||||||||||||||||||||||||||||||
|
|
@@ -1562,6 +1571,62 @@ def _dm_top_level_threads_as_sessions(self) -> bool: | |||||||||||||||||||||||||||||||||
| return True # default: each DM thread is its own session | ||||||||||||||||||||||||||||||||||
| return str(raw).strip().lower() in {"1", "true", "yes", "on"} | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| def _cron_continuable_surface(self) -> str: | ||||||||||||||||||||||||||||||||||
| """Resolve the continuable-cron delivery surface for this platform. | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Values: ``"thread"`` (default — today's behaviour: a continuable cron | ||||||||||||||||||||||||||||||||||
| job opens a dedicated hidden thread and seeds it) or ``"in_channel"`` | ||||||||||||||||||||||||||||||||||
| (deliver FLAT into the channel timeline; the shared-channel session | ||||||||||||||||||||||||||||||||||
| ``(slack, channel_id, None)`` is the continuation surface). Set | ||||||||||||||||||||||||||||||||||
| ``platforms.slack.extra.cron_continuable_surface: in_channel`` in | ||||||||||||||||||||||||||||||||||
| config.yaml. Pair with ``reply_in_thread: false`` so the user's reply | ||||||||||||||||||||||||||||||||||
| is answered flat in the channel and keyed to the same shared session — | ||||||||||||||||||||||||||||||||||
| see ``_warn_if_inchannel_without_flat_reply``. Any unrecognised value | ||||||||||||||||||||||||||||||||||
| coerces to ``"thread"`` (fail safe). | ||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||
| raw = self.config.extra.get("cron_continuable_surface") | ||||||||||||||||||||||||||||||||||
| if raw is None: | ||||||||||||||||||||||||||||||||||
| return "thread" | ||||||||||||||||||||||||||||||||||
| val = str(raw).strip().lower() | ||||||||||||||||||||||||||||||||||
| return "in_channel" if val == "in_channel" else "thread" | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| def _warn_if_inchannel_without_flat_reply(self, team_name: str) -> None: | ||||||||||||||||||||||||||||||||||
| """Warn when ``in_channel`` is set without the required ``reply_in_thread: false`` pairing. | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| The two knobs are orthogonal (D4/D5): ``cron_continuable_surface: | ||||||||||||||||||||||||||||||||||
| in_channel`` skips thread creation on delivery, and ``reply_in_thread: | ||||||||||||||||||||||||||||||||||
| false`` makes the bot answer inbound channel messages flat and key them | ||||||||||||||||||||||||||||||||||
| to the whole-channel session ``(slack, channel_id, None)``. For a | ||||||||||||||||||||||||||||||||||
| continuable in-channel cron to actually continue on a plain reply, BOTH | ||||||||||||||||||||||||||||||||||
| must hold: the seed lands in the shared-channel session, and the reply | ||||||||||||||||||||||||||||||||||
| must resolve to (and be answered in) that same flat session. | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Enforcement is WARN, not hard-require (D5): the misconfiguration fails | ||||||||||||||||||||||||||||||||||
| SAFE — ``in_channel`` without ``reply_in_thread: false`` yields a | ||||||||||||||||||||||||||||||||||
| threaded continuation (≈ today's behaviour), never a dropped/orphaned | ||||||||||||||||||||||||||||||||||
| session — so a config-load rejection would be heavier than warranted | ||||||||||||||||||||||||||||||||||
| and would make the two knobs non-orthogonal. Mirrors the existing | ||||||||||||||||||||||||||||||||||
| connect-time warning pattern (``_warn_if_missing_group_dm_scopes``, | ||||||||||||||||||||||||||||||||||
| ``_warn_if_not_bot_token``). | ||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||
| if self._cron_continuable_surface() != "in_channel": | ||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||
| # reply_in_thread defaults True (legacy: reply in a thread). | ||||||||||||||||||||||||||||||||||
| if self.config.extra.get("reply_in_thread", True): | ||||||||||||||||||||||||||||||||||
| logger.warning( | ||||||||||||||||||||||||||||||||||
| "[Slack] %s: cron_continuable_surface=in_channel is set " | ||||||||||||||||||||||||||||||||||
| "WITHOUT reply_in_thread=false. A continuable in-channel " | ||||||||||||||||||||||||||||||||||
| "cron job will deliver flat, but the bot will still reply " | ||||||||||||||||||||||||||||||||||
| "to your continuation in a thread — so it falls back to a " | ||||||||||||||||||||||||||||||||||
| "threaded continuation (\u2248 default behaviour), not the " | ||||||||||||||||||||||||||||||||||
| "flat channel session you asked for. Set " | ||||||||||||||||||||||||||||||||||
| "platforms.slack.extra.reply_in_thread: false to pair them.", | ||||||||||||||||||||||||||||||||||
| team_name, | ||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||
| except Exception: | ||||||||||||||||||||||||||||||||||
| pass | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| def _resolve_thread_ts( | ||||||||||||||||||||||||||||||||||
| self, | ||||||||||||||||||||||||||||||||||
| reply_to: Optional[str] = None, | ||||||||||||||||||||||||||||||||||
|
|
@@ -2097,10 +2162,10 @@ async def send_image( | |||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| async def _ssrf_redirect_guard(response): | ||||||||||||||||||||||||||||||||||
| """Re-check redirect targets so public URLs cannot bounce into private IPs.""" | ||||||||||||||||||||||||||||||||||
| if response.is_redirect and response.next_request: | ||||||||||||||||||||||||||||||||||
| redirect_url = str(response.next_request.url) | ||||||||||||||||||||||||||||||||||
| if not is_safe_url(redirect_url): | ||||||||||||||||||||||||||||||||||
| raise ValueError("Blocked redirect to private/internal address") | ||||||||||||||||||||||||||||||||||
| from tools.url_safety import redirect_target_from_response | ||||||||||||||||||||||||||||||||||
| redirect_url = redirect_target_from_response(response) | ||||||||||||||||||||||||||||||||||
| if redirect_url and not is_safe_url(redirect_url): | ||||||||||||||||||||||||||||||||||
| raise ValueError("Blocked redirect to private/internal address") | ||||||||||||||||||||||||||||||||||
|
Comment on lines
2163
to
+2168
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 redirect_target_from_response imported but never defined -- breaks Slack send_image redirect guard (bug) In plugins/platforms/slack/adapter.py line 2165, the PR replaces the inline redirect-guard logic with 💡 Suggestion: Revert the slack adapter _ssrf_redirect_guard to the original inline redirect-checking code that works with httpx response objects directly: check response.is_redirect and response.next_request, then extract str(response.next_request.url). This restores the working SSRF redirect guard that was replaced by a call to a non-existent function.
Suggested change
📋 Prompt for AI AgentsIn plugins/platforms/slack/adapter.py lines 2163-2168, revert the _ssrf_redirect_guard inner function to the original inline redirect-checking code. Replace the import of redirect_target_from_response (which does not exist) with direct httpx response attribute checks: check response.is_redirect and response.next_request, then extract str(response.next_request.url). This restores the SSRF redirect validation that was broken by the refactor. |
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| # Download the image first | ||||||||||||||||||||||||||||||||||
| async with httpx.AsyncClient( | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 No warning when DM in_channel cron requires dm_top_level_threads_as_sessions=false (bug)
The _warn_if_inchannel_without_flat_reply method at plugins/platforms/slack/adapter.py line 1593 emits a connect-time warning when cron_continuable_surface=in_channel is set without reply_in_thread=false (channel-level config). However, it does not warn about the DM-level requirement: DM continuation with in_channel cron requires dm_top_level_threads_as_sessions=false. Under the default (true), a DM reply to a flat cron seed creates a per-message session that diverges from the flat seed session key, silently breaking the continuation. The e2e test at tests/manual/cron_inchannel_dm_e2e.py confirms this divergence.
💡 Suggestion: Extend _warn_if_inchannel_without_flat_reply to also check dm_top_level_threads_as_sessions when cron_continuable_surface=in_channel, emitting an additional warning when it defaults to True.
📋 Prompt for AI Agents
In plugins/platforms/slack/adapter.py, method _warn_if_inchannel_without_flat_reply (around line 1612-1627): after the existing reply_in_thread check, add a second check: examine self.config.extra.get('dm_top_level_threads_as_sessions', True) and if True, emit a logger.warning that DM continuations with cron_continuable_surface=in_channel require dm_top_level_threads_as_sessions: false. Direct users to set platforms.slack.extra.dm_top_level_threads_as_sessions: false in config.yaml.