feat(cron/slack): flat in-channel continuable cron delivery surface - #56254
Merged
Conversation
Add a per-platform `cron_continuable_surface` extra key (`thread` default | `in_channel`) so a continuable cron job can deliver FLAT into a Slack channel — no dedicated thread — and still be replied-to. In `in_channel` mode the scheduler skips the thread-open branch (leaves `thread_id=None`); the shipped origin-mirror then seeds the `(slack, chat_id, None)` shared-channel session — the same bucket `reply_in_thread: false` routes inbound channel replies to — so a plain channel reply continues the job in context. Design: specs/cron-inchannel-continuable (D1–D7, F5). Model B (shared-channel session), NOT anchoring to the delivery `ts` — on Slack replying to a specific message IS threading, so a `ts` anchor would only relocate the thread, never deliver true threadless continuable. - gateway/platforms/base.py: `supports_inchannel_continuable` capability flag (default False → unsupported platforms fail SAFE to `thread`). - plugins/platforms/slack/adapter.py: flag=True; `_cron_continuable_surface()` resolver (coerces to the two-value enum); `_warn_if_inchannel_without_flat_reply` connect-time warning (D5: warn, not hard-require — the misconfig fails safe). - gateway/config.py: shared-key bridge line (top-level OR nested config). - cron/scheduler.py: read the key generically from platform config, gate the `in_channel` branch on the adapter capability flag, skip thread-open. No new seed function (reuses the existing mirror — G6). Pairing (docs): `in_channel` + `reply_in_thread: false` + `require_mention: false` (or a free-response channel). Missing `reply_in_thread: false` fails safe to a threaded continuation. Gateway-side config flag — `/restart` to apply; NO Slack app reinstall. Tests (from inside the worktree, PYTHONPATH=$PWD): - +6 cron scheduler tests (in_channel skips thread-open; seeds flat channel session with thread_id=None; thread-mode regression; fail-safe on unsupported platform; value coercion). Prove-fail: removing the `and not in_channel_surface` guard turns the two load-bearing tests RED; restore → GREEN. - +10 slack resolver/capability/warning tests; +2 config-bridge tests. - tests/manual/cron_inchannel_e2e.py: offline E2E driving BOTH real legs (delivery seed + inbound reply keying) → both converge on (slack, C, None). - No regressions: test_slack.py 216 passed alone; broader sweep green (4 pre-existing cross-file-ordering failures reproduce identically on pristine origin/main). Docs: cron.md + slack.md + zh-Hans mirrors of both.
…appends) Live testing exposed a real bug: an in_channel continuable cron delivered flat to the channel (✅) but the reply did NOT continue the job — the bot had no brief in context and confabulated the answer. Root cause: mirror_to_session only APPENDS to a session that already exists (_find_session_id → no-op when none matches); it never CREATEs one. A flat (slack, chat_id, None) row is only created when a human posts a top-level message the bot processes — a cron chat_postMessage delivery never goes through the inbound handler, so the row is absent and the brief is silently dropped. The prior impl relied on the bare mirror (F5/OQ-1 concluded "deletion only" — wrong). Fix: _seed_cron_channel_session mirrors _seed_cron_thread_session — get_or_create_session FIRST (chat_type = "dm" if is_dm else "group", thread_id=None), keyed to the ORIGIN USER'S id, then mirror. The channel session key embeds user_id (…:group:<chat>:<user>), so a system:cron id would key the seed away from the reply; the origin user's id makes seed key == inbound reply key. DM key ignores user_id but needs chat_type=dm to match the prefix. Wired into the in_channel branch after delivery; suppresses the generic mirror to avoid double-write. DM validated (per request): the seeded key equals the inbound DM reply key for a 1:1 DM; continuation works there too. Tests: - Rewrote the in_channel tests to use a real _session_store and the origin user_id; assert get_or_create_session is called with the flat, correctly- keyed source. Prove-fail: (a) reverting the create step and (b) seeding with system:cron each turn a targeted test RED; restore → GREEN. - +2 direct _seed_cron_channel_session unit tests asserting the KEY-MATCH invariant (seed key == inbound reply key) via build_session_key, for both channel and DM. - Rewrote tests/manual/cron_inchannel_e2e.py to drive a REAL SessionStore + real mirror_to_session + real _find_session_id + real build_session_key (no session-layer mocks — the old mocked E2E is exactly why the bug shipped). Asserts the brief lands in the transcript and the reply resolves to the same session, for BOTH channel and 1:1 DM. Full relevant sweep: 283 passed.
Live DM testing showed a reply to a DM cron brief did NOT continue the job. Root cause: for a 1:1 DM the governing knob is dm_top_level_threads_as_sessions (default True), NOT reply_in_thread / cron_continuable_surface. Under the default, each top-level DM keys to a per-message session (…:dm:<chat>:<ts>), so a reply mints a new ts and can never converge with the flat …:dm:<chat> session the cron seed creates. A 1:1 DM has no thread-vs-timeline split, so "in_channel" has no coherent meaning for a DM — cron_continuable_surface is a channel concept and is a no-op for DMs. DM continuation is governed entirely by dm_top_level_threads_as_sessions: - false → all top-level DMs share …:dm:<chat> → seed + reply converge → works - true (default) → per-message sessions → no continuation (cron or interactive) Option A (chosen): document the requirement; no code change (the flat-DM seed from the prior commit already lands correctly when the knob is false). Adds a ":::note 1:1 DMs" admonition to cron.md + the zh-Hans mirror. Verification (real inbound handler, not a hard-coded assumption — the mistake that made the earlier DM E2E falsely pass): tests/manual/cron_inchannel_dm_e2e.py drives the REAL _handle_slack_message for a top-level DM under both knob values and asserts false→converges (…:dm:D_TESTDM == seed), true→diverges (…:dm:D_TESTDM:<ts>). See decisions.md D9.
The in_channel surface DOES add a seed: _seed_cron_channel_session CREATES the flat (platform, chat_id, None) session and mirrors the brief into it, because mirror_to_session only APPENDS to an existing session and the flat channel row is otherwise absent for a chat_postMessage delivery. Correct the scheduler thread-skip comment and the test class docstring, which still described the earlier 'let the existing mirror seed it' design.
alt-glitch
added
type/feature
Collaborator
1 task
14 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 #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 #56096. Original contributor @benbarclay's commit authorship preserved via rebase-merge.
Nous Research