feat(cron/slack): flat in-channel continuable cron delivery surface - #260
Conversation
|
Review Complete Files Reviewed: 13 By Severity:
PR #260 introduces continuable cron surfaces for Slack (in_channel, DM, continuable thread) but ships with a critical import-to-nonexistent-function bug that breaks Slack image-send SSRF redirect guarding, plus a documentation config-path error and a missing DM config warning. Files Reviewed (13 files) |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
Risk: 🟠 High (68/100) — 1 high finding, 3 medium · 1389 LOC across 13 files
Assessment: request_changes
Critical Issue
redirect_target_from_responsedoes not exist (finding-001):plugins/platforms/slack/adapter.pyline 2163 imports a function fromtools/url_safetythat was never defined. This replaces previously-working inline SSRF redirect validation with guaranteedImportErroron every image-send response event hook — breaking the redirect guard AND image delivery simultaneously. The same broken import pre-exists ingateway/platforms/base.pybut this PR introduces a new broken call site.
Medium Issues
-
Wrong dotenv mock in test (finding-002):
tests/cron/test_scheduler.pyline 1944 mocksdotenv.load_dotenv(old function) but production code now callshermes_cli.env_loader.load_hermes_dotenv. The mock is silently ineffective. -
Wrong config path in docs (finding-003):
website/docs/user-guide/features/cron.mddocumentsdm_top_level_threads_as_sessionsunder the top-levelslack:block, but the key lives atplatforms.slack.extra.dm_top_level_threads_as_sessionsand is not bridged. Users following docs get silent defaulttrue. -
Missing DM config warning (finding-004):
_warn_if_inchannel_without_flat_replywarns about missingreply_in_thread: falsefor channels but omits the parallel DM requirementdm_top_level_threads_as_sessions: false. DM continuation silently breaks under default config.
Cross-file Architecture
The PR sprawls across 13 files touching scheduler delivery routing, Slack adapter surface negotiation, gateway config bridging, and documentation. The core feature (continuable cron surfaces) is well-structured, but the SSRF guard refactor and config-path documentation errors need fixing before merge.
| 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") |
There was a problem hiding this comment.
🟠 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 from tools.url_safety import redirect_target_from_response. The function does not exist in tools/url_safety.py or anywhere else. The old code correctly checked response.is_redirect and response.next_request before accessing response.next_request.url. The new code will raise ImportError every time the httpx response event hook fires during image download in Slack, crashing the image send and falling through to text-only fallback -- simultaneously disabling SSRF redirect validation. The same missing function is also imported in gateway/platforms/base.py line 549 (pre-existing bug, not introduced here), but this PR introduces a NEW broken call site that replaces previously-working code.
💡 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.
| 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") | |
| 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") |
📋 Prompt for AI Agents
In 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.
|
|
||
| with patch("cron.scheduler._hermes_home", tmp_path), \ | ||
| patch("cron.scheduler._resolve_origin", return_value=None), \ | ||
| patch("dotenv.load_dotenv"), \ |
There was a problem hiding this comment.
🟡 Test mocks wrong dotenv function (load_dotenv instead of load_hermes_dotenv) (bug)
In tests/cron/test_scheduler.py line 1944, the test test_fallback_chain_merges_providers_and_legacy_model patches dotenv.load_dotenv, but the production code in cron/scheduler.py lines 2447-2452 calls hermes_cli.env_loader.load_hermes_dotenv and hermes_cli.env_loader.reset_secret_source_cache. All other tests in the file use the new mocking pattern; this test was missed. The mock is a no-op, allowing the real functions to execute unmocked, which can leak side effects and affect global state between tests.
💡 Suggestion: Replace patch('dotenv.load_dotenv') with patch('hermes_cli.env_loader.load_hermes_dotenv') and add patch('hermes_cli.env_loader.reset_secret_source_cache') to match all other tests in the file.
📋 Prompt for AI Agents
In tests/cron/test_scheduler.py, test test_fallback_chain_merges_providers_and_legacy_model at line 1944: replace patch('dotenv.load_dotenv') with patch('hermes_cli.env_loader.load_hermes_dotenv') and add patch('hermes_cli.env_loader.reset_secret_source_cache') in the adjacent line, matching the pattern used by every other test in the file (60+ occurrences).
| the separate, pre-existing knob **`slack.dm_top_level_threads_as_sessions`**: | ||
|
|
||
| - **`false`** — all top-level DMs share one rolling DM session, so a continuable | ||
| cron brief and your reply land in the **same** session and the job continues in | ||
| context. This is what you want for continuable cron in a DM. | ||
| - **`true`** (default) — each top-level DM message is its own session, so a reply | ||
| to a delivered brief starts a *fresh* session that has no record of the brief. | ||
| Continuation does not work in this mode (for cron or any other flat delivery). | ||
|
|
||
| So for a continuable cron job delivered to a 1:1 DM, set | ||
| `slack.dm_top_level_threads_as_sessions: false`. `cron_continuable_surface` is |
There was a problem hiding this comment.
🟡 Documentation uses wrong config path for dm_top_level_threads_as_sessions (bug)
Both English (website/docs/user-guide/features/cron.md lines 377, 387) and Chinese (website/i18n/zh-Hans/.../cron.md lines 382, 390) documentation reference the config key as slack.dm_top_level_threads_as_sessions, implying it works under a top-level slack: YAML block like the other keys shown nearby (cron_continuable_surface, reply_in_thread, require_mention). However, dm_top_level_threads_as_sessions is NOT bridged in gateway/config.py -- it is only read from self.config.extra in the adapter. The correct path is platforms.slack.extra.dm_top_level_threads_as_sessions. Users following the documented pattern will silently get the default true behavior.
💡 Suggestion: Update both the English and Chinese documentation to use the canonical full path platforms.slack.extra.dm_top_level_threads_as_sessions, and add a note that this key is NOT available as a top-level slack: shortcut (unlike the other three keys).
📋 Prompt for AI Agents
In website/docs/user-guide/features/cron.md, change the two references to slack.dm_top_level_threads_as_sessions (lines 377 and 387) to platforms.slack.extra.dm_top_level_threads_as_sessions and add a clarifying note that this key lives under platforms.slack.extra, not the top-level slack: block. Apply the same change to website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/user-guide/features/cron.md lines 382 and 390.
| 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 |
There was a problem hiding this comment.
🟡 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.
1 participant
Summary
Adds a per-platform
cron_continuable_surfaceconfig key so a continuable cron job can deliver flat into a Slack channel — no dedicated hidden thread — and still be replied-to and continued in context. Salvage of NousResearch#56096 by @benbarclay, rebased onto currentmain.Today continuable cron (
cron.mirror_delivery/ per-jobattach_to_session) is thread-preferred: on any thread-capable platform it unconditionally mints a hidden handoff thread. This adds a "continuable but flat" mode.How it works
platforms.<p>.extra.cron_continuable_surfacetakes"thread"(default, byte-identical to today) or"in_channel". Inin_channelmode the scheduler skips the thread-open branch (leavesthread_id=None) so the delivery posts flat, then_seed_cron_channel_sessioncreates the flat shared-channel session(slack, chat_id, None)and mirrors the brief into it — the same bucketreply_in_thread: falseroutes inbound channel replies to. A plain channel reply resolves to that session with the brief in context.The scheduler reads the key generically from platform config; the
in_channelbranch is gated on the base-adapter capability flagsupports_inchannel_continuable(Slack =True). Any platform without an implementation fails safe tothreadwith a debug log — no Slack-only special-case in core.Changes
gateway/platforms/base.py—supports_inchannel_continuablecapability flag (defaultFalse).plugins/platforms/slack/adapter.py— flagTrue;_cron_continuable_surface()resolver; connect-time warning whenin_channelis set withoutreply_in_thread: false.gateway/config.py— shared-key bridge line.cron/scheduler.py— resolve the surface generically, gate thein_channelbranch on the capability flag, skip thread-open, and_seed_cron_channel_sessionto create + seed the flat session.cron.md+slack.md(+ zh-Hans mirrors).Salvage follow-up (ours)
Corrected stale "no new seed code (G6)" comments in the scheduler and the test class docstring — the earlier "let the existing mirror seed it" design was superseded during implementation (the mirror only appends to an existing session; the flat channel row is absent for a
chat_postMessagedelivery, so the seed must create it first).Validation
tests/cron/test_scheduler.pytests/gateway/test_slack_cron_continuable_surface.pytests/gateway/test_slack_mention.pyManual offline E2E scripts under
tests/manual/(notest_funcs,__main__-guarded → not collected by CI). Base 0 commits behindmain; diff shows only the PR's 13 files.Infographic
Salvaged from NousResearch#56096. Original contributor @benbarclay's commit authorship preserved via rebase-merge.
Nous Research
Mirror-of: NousResearch#56254
NousResearch#56254