feat(cron/slack): flat in-channel continuable cron delivery surface - #56096
Closed
benbarclay wants to merge 4 commits into
Closed
feat(cron/slack): flat in-channel continuable cron delivery surface#56096benbarclay wants to merge 4 commits into
benbarclay wants to merge 4 commits into
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.
alt-glitch
added
type/feature
…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.
…ntinuable # Conflicts: # website/docs/user-guide/messaging/slack.md # website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/user-guide/messaging/slack.md
Contributor
|
Merged via #56254 — your commits were rebased onto current Salvage was minimal: resolved additive doc conflicts (main added |
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 thread — and still be replied-to and continued in context.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 (cron/scheduler.py). There was no "continuable but flat" mode. This adds one.How it works
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 — and the shipped origin-mirror then seeds the(slack, chat_id, None)shared-channel session, the exact bucketreply_in_thread: falsealready routes inbound channel replies to. A plain channel reply resolves to that session with the brief in context. No new seed code — it reuses the existing mirror.This is model B (shared-channel session), deliberately not anchoring the seed to the delivery message's
ts: on Slack, replying to a specific message is threading, so atsanchor would only relocate the thread to first-reply-time, never deliver true threadless-continuable.Config (the pairing)
reply_in_thread: falseis required so the reply is answered flat and keyed to the seeded channel session. Missing it fails SAFE — you get a threaded continuation (≈ today), never a dropped reply — and the gateway logs a warning at startup (warn, not hard-reject: the two knobs stay orthogonal).require_mention: false(or a free-response channel) lets a plain reply continue the job; otherwise you'd have to@-mention on each reply. (Long-standing orthogonal channel knob — surfaced in docs.)reply_in_thread: falseusers already accept. Use the defaultthreadsurface when you want each delivery isolated.Generic key, Slack first consumer
The scheduler reads the key generically from platform config; the
in_channelbranch is gated on a 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.Files
gateway/platforms/base.py—supports_inchannel_continuablecapability flag (defaultFalse).plugins/platforms/slack/adapter.py— flagTrue;_cron_continuable_surface()resolver;_warn_if_inchannel_without_flat_reply()connect-time warning.gateway/config.py— shared-key bridge line (top-level or nested config).cron/scheduler.py— resolve the surface generically, gate thein_channelbranch on the capability flag, skip thread-open.cron.md+slack.md(+ zh-Hans mirrors of both).Testing
Run from inside the worktree (
PYTHONPATH=$PWD):in_channelskips thread-open; seeds the flat channel session withthread_id=None;thread-mode regression; fail-safe on an unsupported platform; value coercion.and not in_channel_surfaceguard turns the two load-bearing tests RED; restore → GREEN.tests/manual/cron_inchannel_e2e.py) — drives both real legs (delivery seed + inbound reply keying) and asserts they converge on(slack, C, None).test_slack.py216 passed alone; broader sweep green. (4 pre-existing cross-file-ordering failures intest_slack.pyreproduce identically on pristineorigin/main.)Deploy note
Gateway-side config flag — a
/restartpicks it up. No Slack app reinstall needed (contrast a manifest/scope change).Infographic