fix(relay): Slack DM-root prompts + flat-DM edit-streaming (native _resolve_thread_ts parity) - #72103
Conversation
|
CI triage: the attribution failure is branch-specific and actionable. Both PR commits use |
alt-glitch
added
type/bug
…thread_ts parity) On the relay lane a Slack DM's streamed reply was sent with reply_to=the triggering message ts; the connector maps a raw reply_to to a Slack thread_ts, so the DM reply posted threaded under the user's message and lost progressive edit-streaming (flat reply, no thinking status). Native SlackAdapter already drops that synthetic DM self-anchor when reply_in_thread is off; the relay lane had no equivalent. Track chat_type per chat in _capture_scope; add _resolve_reply_to_for_send so a Slack DM with no real thread_id/thread_ts drops reply_to (and the mirrored reply_to_message_id) and posts flat at the DM root, edit-streaming its own ts. Never invents a thread_id; real threads and channel autoThread keep reply_to; non-DM/non-Slack untouched. Adapted to main's phase-3 prompt architecture.
A prompt (approval/clarify) is emitted in reply to the triggering inbound event, so its metadata carries that event's synthetic DM thread anchor; forwarded to the connector it threads the Block Kit prompt under the user's message instead of posting flat at the DM root. Main routes all prompts through the single _send_prompt prompt-op choke point, so strip the synthetic DM thread anchor there via _strip_synthetic_dm_thread — preserving real threads (distinct thread_id), tenant scope (scope_id/slack_team_id), and non-DM/non-Slack chats. Preserves main's hp1 prompt-codec; no competing ap:/cl: encoding.
7f552de to
08d6779
Compare
…ootgun) check-windows-footguns (blocking CI) flagged a bare Path.write_text() without encoding= at scripts/tool_search_livetest2.py:190, which uses the platform locale encoding on Windows. Pin utf-8. Pre-existing on main; unblocks the required-checks gate for this PR.
Slack Block Kit buttons only support style primary (green) / danger (red) / default (white). The relay approval + slash-confirm prompts emitted an invalid style 'success' (Slack silently drops it → white/stroke button) and baked emoji into the labels (non-native). Native Slack Hermes uses plain labels with primary/danger. Map to valid styles (once→primary, deny/cancel→danger, session/always→default) and drop the emoji from labels. The connector already compensates success→primary, but emitting valid values at the source is correct and removes the fragile dependency on that compensation.
…-derivation (QA-5) The threading mode (flat vs thread-per-message) is decided once, in run.py's _resolve_progress_thread_id (reply_in_thread knob), and encoded in the metadata stamp: flat => no thread_id, threaded => thread_id for the turn (first turn: == message_id, the synthetic root IS the thread). _strip_synthetic_dm_thread re-derived the mode with an unconditional thread_id == message_id strip, exiling approval/clarify cards (and their resolved-state swaps) to the DM root while progress bubbles honoured the thread (2026-07-27 mixed-placement report). Trust the stamp instead; flat mode is unaffected because flat metadata never carries an anchor.
…reply_to strip on reply_in_thread (QA-6) _resolve_reply_to_for_send dropped the triggering-ts reply_to on every Slack DM with no metadata thread_id. But the final-reply lane (platforms/ base.py) builds metadata from source.thread_id only — None for a top-level DM — so in thread-per-message mode that reply_to is the final reply's ONLY threading signal, and stripping it exiled the final message to the DM root while progress bubbles stayed threaded (sibling of the QA-5 prompt bug). Mirror native _resolve_thread_ts: suppress the synthetic anchor only when platforms.slack.extra.reply_in_thread=false. Flat mode behavior unchanged; real threads and channels unchanged.
…s_text, carry live per-tool phrase on typing frames (QA-1)
Native Slack shows dynamic assistant-status text ('Finding answers…',
'is running pytest…') because SlackAdapter sets supports_status_text=True
and renders the set_status_text() phrase in send_typing. The relay lane
advertised nothing, so run.py's live-status lane never fed it phrases and
the connector fell back to the static default.
- supports_status_text: descriptor-gated property (Slack only; other
fronted platforms keep textless bubbles)
- send_typing: carry the stashed phrase as the typing op's content; omit
when unset (empty string is Slack's explicit clear, reserved for
stop_typing). Connector already renders content via
assistant.threads.setStatus (NousResearch#154).
… the anchor from the inbound ts (QA-1) Slack's thinking-status line (thread replies footer, plain chat:write — no assistant scopes needed) is thread-only: the connector's typing case no-ops without thread_ts. The typing lane's metadata has no anchor for a top-level DM (base.py builds from source.thread_id = None), so every status heartbeat was silently dropped — the trace showed typing frames with meta_keys=['user_id'] only. Cache the triggering message ts per chat on inbound (_capture_scope) and synthesize metadata.thread_id on send_typing/stop_typing in thread-per-message mode, mirroring native send_typing's _resolve_thread_ts(metadata.message_id). Flat mode unchanged (NousResearch#18859); real-thread metadata wins over the cache; the clear frame targets the same synthesized thread so the status never sticks.
…d_id on Slack sends (QA-7) The connector's Slack sender threads on metadata ONLY: threadTs() reads metadata.thread_id/thread_ts and never the frame's reply_to. base.py's final-reply lane (and its stream-fallback 'first response' resend) builds metadata from source.thread_id — None for a top-level DM — so its sends carried reply_to as the sole threading signal and posted to the home channel (2026-07-27 post-approval report; the 15:17:03 frame showed meta_keys=['notify','user_id']). After the QA-6 mode gate keeps the anchor, copy it into metadata.thread_id so the wire carries the signal where the connector reads it. Flat mode unaffected (anchor already nulled); explicit thread metadata wins; non-Slack untouched.
|
Correctness batch from live QA on the staging deployment (2026-07-27) — five commits, all fixing flat-DM-era assumptions in this PR's own lanes that broke
All five verified live on the staging stack (thread-per-message, no-assistant manifest): thinking-status renders, approval card + resolved swap + final reply all land in the per-message thread, flat mode regression-guarded. Relay suite: 288 passed on this branch (mode-aware contracts both directions). Note: cherry-picked from a staging branch based on current main; the |
…bound ts as session thread (QA-3)
A 2nd top-level DM while a turn was in flight resolved to the SAME
session key and steered the running turn ('Redirected current run')
instead of starting its own. Native SlackAdapter stamps thread_ts =
event.thread_ts or ts on EVERY inbound, so build_session_key isolates
each top-level message; the connector normalizes top-level messages with
thread_id=null and the relay lane never reproduced the stamp.
_stamp_slack_session_thread applies native parity on the inbound bridge:
top-level Slack message + thread-per-message mode => source.thread_id =
its own ts (fresh session, parallel turns). Real thread replies and flat
mode untouched (flat keeps the shared rolling DM session on purpose).
Also introduces the enterprise config shape for relay-fronted Slack:
platforms.relay.extra.slack.<subset of native Slack fields> (nested
object wins; legacy flat extra.reply_in_thread still honoured). All
reply_in_thread reads (send/typing/stop_typing/run.py progress) now
route through one resolver.
…, replies stay flat (QA-8) Victor's correction: flat DMs CAN have a live thinking status. setStatus on the triggering message's ts renders '… thinking'/per-tool phrases in that message's thread-footer space and clears without leaving a message artifact. Native suppresses this because ITS reply routing could inherit the activated thread; the relay lane's flat-mode sends strip their anchors explicitly (QA-6/7), so the status anchor cannot leak into reply placement — proven by the new leak-guard test. send_typing/stop_typing now anchor the status in flat mode too, gated by platforms.relay.extra.slack.flat_dm_status (default ON; false restores the fully anchorless posture). Thread mode unchanged.
|
+2 commits from the final QA round (both live-verified on staging 2026-07-27):
Full batch now: QA-5/6/1a/1b/7/3/8 — seven commits, 295 relay tests green, every one verified on the live staging fleet. This completes the native-parity contract in BOTH delivery modes. |
…tional; add relay docs page flat_dm_status was speculative config (rubric violation): no user wants 'make my agent look dead', and the only real consumer of status suppression was native's placement-contamination guard — which the relay lane handles structurally (QA-6/7 send-side anchor strip, leak-guard test), not via preference. Status now anchors whenever an inbound ts exists, in both modes. Docs: new website/docs/user-guide/messaging/relay.md — enterprise-only relay lane page documenting the platforms.relay.extra.<platform> subset shape (nested wins, flat fallback), the Slack reply_in_thread control, and always-on liveliness. Kept out of the native slack.md on purpose: relay controls are not Slack config.
…tract Relocate the platforms.relay.extra.<platform> documentation from a new user-guide page into docs/relay-connector-contract.md (the existing canonical relay doc, already linked from gateway-internals) as §8. The relay lane is an enterprise-only component: it gets minor coverage in the developer-facing contract doc, not a prominent user-guide page, and no links to private components.
Review: changes requestedPosted by Ben's Hermes Agent (automated review on Ben's behalf, using his account). Reviewed the full diff against 1.
|
| Commit | Change |
|---|---|
b493bf63 |
advertises supports_status_text for relay-fronted Slack |
467534b4 |
typing/status synthesizes a thread anchor from the inbound ts |
71d5c47e |
_stamp_slack_session_thread — per-message sessions for fronted DMs |
a51a17ab |
promotes surviving reply_to into metadata.thread_id |
9864e00f / 85a75f31 |
flat-DM liveliness; drops the flat_dm_status knob |
42e4f70e |
rewrites approval button labels and styles |
daefa8c3 |
adds a new §8 to the connector contract doc |
The new platforms.relay.extra.slack.reply_in_thread config surface and the run.py duck-typed _effective_reply_in_thread hook are both user-visible and both undocumented in the body. Please bring the description in line with what actually shipped.
3. No CI has run on this PR
$ gh pr checks 72103
no checks reported on the 'feat/relay-slack-blockkit-native-parity' branch
mergeable: UNKNOWN, statusCheckRollup: 0. It's a fork PR (victor-kyriazakos/hermes-agent), so workflows need maintainer approval to run — nothing has been validated by the gate.
For what it's worth, locally the suite is green on the branch head via the canonical runner:
$ scripts/run_tests.sh tests/gateway/relay/
=== Summary: 27 files, 295 tests passed, 0 failed in 4.6s ===
That's encouraging but it isn't the gate — this needs a real CI run before it can be assessed.
4. Merge conflicts against current main
git merge-tree reports three conflicting files: gateway/relay/adapter.py, gateway/run.py, and scripts/tool_search_livetest2.py. Please merge latest main into the branch (not rebase).
Also worth addressing
Drop the scripts/tool_search_livetest2.py commit (e28665837). It's unrelated to the Slack relay lane, and the identical fix is already on main — line 190 there already reads out_path.write_text(json.dumps(rec, indent=1), encoding="utf-8"). It contributes nothing but one of the three merge conflicts above.
12 QA-N references in shipped source. gateway/relay/adapter.py carries QA-1, QA-3, QA-5, QA-6, QA-7 markers throughout its comments. These are internal QA-campaign tracker IDs — they mean nothing to a future reader of this file. Describe the behavior instead. (The contract doc and run.py are already clean, so this is just adapter.py.)
Session-keying parity gap worth a second look. _stamp_slack_session_thread makes every top-level relay Slack DM key a fresh session. Native has the same default, but gates it behind an explicit escape hatch:
# plugins/platforms/slack/adapter.py
def _dm_top_level_threads_as_sessions(self) -> bool:
"""... Set platforms.slack.extra.dm_top_level_threads_as_sessions
to false in config.yaml to revert to the legacy behavior ..."""The relay version has no equivalent opt-out — it's gated only on reply_in_thread, which also controls reply placement. So an operator who wants threaded replies and one rolling DM session can't express that, whereas on native they can. Since this PR is explicitly framed as native parity, either add the knob or note in the contract doc why the relay lane deliberately couples the two.
Summary
The core streaming fix is good and I'd like to see it land. Blocking on: the dead-code decision (1), a description that matches the branch (2), an actual CI run (3), and a merge from main (4).
alt-glitch
added
sweeper:risk-message-delivery
…indows footgun)" This reverts commit e286658.
…py anchor-suppression boundary Review finding (2026-07-28): every path through _strip_synthetic_dm_thread returned metadata unmodified — the actual strip was removed when prompts switched to trusting the run.py thread stamp, leaving a 50-line no-op and four tests that passed against it (verified by reviewer's negative control). - delete the function + its _send_prompt call site (verbatim pass-through with a pointer comment to the single mode authority) - rewrite the three pass-through tests as end-to-end placement contracts (forward run.py's stamp untouched) - NEW boundary tests pinning run.py._resolve_progress_thread_id itself: flat mode suppresses the synthetic self-anchor / preserves real threads; thread mode keeps the first-turn self-anchor. This is the cross-module coupling the review flagged as unpinned — if the upstream suppression regresses, these fail instead of prompts silently threading.
…riptions Review finding: QA-1/3/5/6/7 are internal campaign tracker ids meaning nothing to future readers of this file. Comments now describe the behavior (status thread targeting, metadata-only threading, session-keying parity) instead of citing the tracker. Comment-only change.
…ession-keying parity Review finding: native gates per-message DM sessions behind platforms.slack.extra.dm_top_level_threads_as_sessions; the relay lane coupled session keying to reply_in_thread alone, so 'threaded replies + one rolling session' was expressible on native but not here. Adds the same knob to the relay subset (platforms.relay.extra.slack. dm_top_level_threads_as_sessions, default true = per-message sessions, unchanged behavior). false keeps thread-per-message reply placement but skips the session stamp — one rolling DM session, legacy steer posture. TDD: opt-out + default-unchanged tests written first.
…kkit-native-parity # Conflicts: # gateway/relay/adapter.py
|
@benbarclay All four blockers + the three advisories addressed. Branch is maintained merge-only from here per your ask — today's remediation lands as appended commits (
One nit from your table for the record: the button-labels commit ( CI approval is yours when ready — the branch tip is |
Re-review: 4 blockers cleared, 3 new findingsPosted by Ben's Hermes Agent (automated review on Ben's behalf, using his account). Verified every claim against the branch tip ( Previous blockers — all cleared
Advisories: A (livetest2) reverted, nets to zero diff. C (session knob) added. B (QA-N scrub) partially done — see finding 3. Canonical runner re-run on the merge tip in a clean worktree: Finding 1 — the media lane bypasses both threading fixes (blocking)
Probe output against the real Both modes are wrong, in opposite directions — the two failure shapes this PR set out to eliminate on the text lane, still live on the media lane. Per the contribution rubric — "fixes the whole bug class, sibling call paths included" — the anchor resolution should move into one shared helper that both Finding 2 — the new knob doesn't coerce like the native one it mirrorsNative: raw = self.config.extra.get("dm_top_level_threads_as_sessions")
if raw is None:
return True
return str(raw).strip().lower() in {"1", "true", "yes", "on"}Relay uses bare A PR framed as 1:1 native parity, adding an escape hatch whose entire purpose is being switched off, silently ignores the off switch when the value is a quoted string. Finding 3 — the QA-N scrub only covered
|
alt-glitch
removed
the
needs-decision
The DM thread-anchor contract was resolved only in send(). _send_media() —
backing send_image, send_image_file, send_voice, send_video and send_document
— passed reply_to straight to the frame and never touched metadata, so
attachments egressing through the same connector-side Slack sender got both
failure shapes this branch set out to remove:
flat mode → reply_to survives, the image threads UNDER the user's DM
message (the original reported symptom)
thread mode → no metadata.thread_id, and threadTs() never reads reply_to,
so the image lands in the home channel instead of the
per-message thread
Both are reachable: gateway/run.py delivers agent artifacts through
send_voice/send_document.
Extract the three steps that must always happen together (mode gate, mirrored
reply_to_message_id strip, metadata promotion) into
_apply_slack_thread_anchor and route BOTH lanes through it, so text and media
cannot drift again. The media lane copies caller metadata rather than mutating
it — these helpers are called in loops with a shared mapping.
Also fold send_typing/stop_typing's duplicated status-anchor blocks into
_with_status_thread_anchor. They had already drifted (stop_typing omitted the
platform check) and the clear must target the thread the heartbeat set or the
status line sticks until Slack's own timeout.
Tests: media lane pinned in both modes plus the channel and
no-caller-mutation cases; verified as real by reverting the fix and watching
them fail.
… does
Both relay Slack knobs read their value through bool(), while the native
adapter they mirror uses str(raw).strip().lower() in {"1","true","yes","on"}.
A YAML-quoted string diverges:
dm_top_level_threads_as_sessions: "false" → relay True, native False
Non-empty strings are truthy, so the escape hatch is silently ignored in
exactly the shape an operator writes to switch it OFF. reply_in_thread has the
same defect and gates reply placement, session keying and run.py's progress
resolver, so one quoted "false" misfires three ways.
Route both through a shared _coerce_flag mirroring native's predicate. Real
booleans pass through untouched; None falls back to the default. Contract §8
documents the accepted spellings.
Tests: both knobs parametrized over the true/false spellings native accepts,
plus the absent-key default.
The earlier scrub covered adapter.py; nine internal QA-campaign tracker IDs remained in the two new test files, including a module docstring and an assertion message. They mean nothing to a future reader — describe the behavior instead. Comments only, no assertion changes.
Fixes pushed — all three findings addressedPosted by Ben's Hermes Agent (automated, using Ben's account). Rather than send this back for another round-trip, I pushed the fixes directly to the branch (
|
alt-glitch
added
the
sweeper:risk-session-state
|
Thanks so much, both for the fixes and for the general-practices seeds. I reviewed your three commits as requested: re-ran the canonical runner on the tip (323 passed, 0 failed), verified the QA-N grep is clean across the full relay surface, and ran your own mutation trick against you: stubbed The negative-control and sibling-path discipline from your reviews has been absorbed into our dev workflow going forward. Appreciated. |
…ay-slack-blockkit-native-parity fix(relay): Slack DM-root prompts + flat-DM edit-streaming (native _resolve_thread_ts parity)
…ay-slack-blockkit-native-parity fix(relay): Slack DM-root prompts + flat-DM edit-streaming (native _resolve_thread_ts parity)
5 participants
What this PR is
Native-parity for Team-Gateway-fronted Slack DMs (the relay lane): streamed replies, rich thinking-status, prompt/approval placement, and session keying all behave exactly as native Slack Hermes. Started as the DM-root streaming fix; grew through a live staging QA campaign (2026-07-26/27, all findings live-verified on a 4-agent fleet) and a review pass (2026-07-28).
Commits (18 on the branch — grouped by concern)
Core streaming fix (original thesis)
15d65da5astream Slack DM replies flat at DM root —_resolve_reply_to_for_sendmirrors native_resolve_thread_ts08d67792apost clarify/approval prompts at DM root, not in a synthetic threadQA correctness batch (live-verified on staging)
95103db64prompts trust the run.py thread stamp (no adapter-side re-derivation)be9de3196final DM reply honors thread-per-message modeb493bf63cadvertisesupports_status_textfor fronted Slack → run.py feeds live per-tool phrases467534b43typing/status synthesizes its thread anchor from the inbound tsa51a17ebefallback resend promotes survivingreply_tointometadata.thread_id(the connector threads on metadata ONLY)71d5c47e2per-message sessions for fronted DMs — native inbound-ts stamp parity (2nd message runs parallel instead of pre-empting)9864e00fbflat-DM liveliness: status anchors to the triggering ts; replies stay flat (leak-guard tested)42e4f70eenative approval button labels/styles (Allow Once / Allow Session / Always Allow / Deny)Refinements
85a75f315drop the speculativeflat_dm_statusknob — liveliness is unconditionaldaefa8c34contract doc §8: gateway-side platform behavior controlsReview remediation (2026-07-28, addresses @benbarclay)
a09015d31revert the unrelated livetest2 commit (identical fix already on main)09c4a1d34remove dead_strip_synthetic_dm_thread; pin the run.py anchor-suppression boundary with tests3e628edebreplace internal QA-N tracker markers with behavior descriptions277fc97a0dm_top_level_threads_as_sessionsescape hatch (native session-keying parity)dd866eef3mergeorigin/main(per review: merge, not rebase; conflicts resolved semantically)User-visible surface (new)
Config subset for the relay lane, under
platforms.relay.extra.slack(nested wins → legacy flat key fallback → default). Documented indocs/relay-connector-contract.md§8:reply_in_threadtruedm_top_level_threads_as_sessionstruePlus a duck-typed
_effective_reply_in_thread()hook consumed byrun.py's progress-thread resolver — the threading MODE is decided in exactly one place and expressed to the connector as frame metadata.Testing
scripts/run_tests.sh tests/gateway/relay/→ 28 files, 306 tests, 0 failed on the merge tip. New coverage includes the run.py↔adapter boundary pins (flat-mode anchor suppression / thread-mode stamp pass-through), session-keying distinctness, the flat-mode leak guard, and the knob default/opt-out contracts. Every behavior change was additionally live-verified on the Coatue staging fleet (Railway, 4 agents + connector).Supersede note (updated)
The branch originally carried a fan-salvage analysis; that stands. The two revert-shaped commits (
e28665837+a09015d31) cancel to zero net diff — kept as history because this branch is maintained merge-only per review.