From 95103db64552f05da214eff7d97191d8530534f4 Mon Sep 17 00:00:00 2001 From: Victor Kyriazakos Date: Mon, 27 Jul 2026 13:50:13 +0000 Subject: [PATCH] =?UTF-8?q?fix(relay):=20prompts=20trust=20the=20run.py=20?= =?UTF-8?q?thread=20stamp=20=E2=80=94=20no=20self-anchor=20re-derivation?= =?UTF-8?q?=20(QA-5)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- gateway/relay/adapter.py | 28 +++-- .../relay/test_relay_slack_prompt_dm_root.py | 104 ++++++++++-------- 2 files changed, 77 insertions(+), 55 deletions(-) diff --git a/gateway/relay/adapter.py b/gateway/relay/adapter.py index e50b9771ed5..57ad659d6d5 100644 --- a/gateway/relay/adapter.py +++ b/gateway/relay/adapter.py @@ -1301,17 +1301,23 @@ class RelayAdapter(BasePlatformAdapter): thread_id = metadata.get("thread_id") if not thread_id: return metadata - # A real thread carries a thread_id distinct from the triggering message - # ts (run.py stamps that ts as metadata["message_id"] on Slack). Only the - # synthetic self-anchor (thread_id == that ts, or no distinguishing anchor - # present) is stripped; a genuine thread is honoured. - anchor = metadata.get("message_id") - if anchor is not None and str(thread_id) != str(anchor): - return metadata - cleaned = dict(metadata) - cleaned.pop("thread_id", None) - cleaned.pop("thread_ts", None) - return cleaned + # Trust the run.py stamp (QA-5). The threading MODE is decided in ONE + # place — run.py's _resolve_progress_thread_id, which reads + # platforms.slack.extra.reply_in_thread: + # * flat mode (reply_in_thread=false): the synthetic self-anchor is + # suppressed THERE, so prompt metadata arrives with NO thread_id and + # this helper is a no-op — the card posts flat at the DM root; + # * thread-per-message mode (default): metadata.thread_id is stamped + # for the whole turn, and on the FIRST turn it legitimately equals + # the triggering message's ts (the synthetic root IS the thread). + # The previous unconditional thread_id == message_id strip re-derived + # the mode here and got it wrong for thread-per-message: the approval + # card (and its resolved-state swap) was exiled to the DM root while + # progress bubbles honoured the thread (2026-07-27 mixed-placement + # screenshot). Mirror native SlackAdapter._resolve_thread_ts, which + # only performs the self-anchor strip when reply_in_thread=false — a + # state this lane never sees with an anchor present, per the above. + return metadata async def _send_prompt( self, diff --git a/tests/gateway/relay/test_relay_slack_prompt_dm_root.py b/tests/gateway/relay/test_relay_slack_prompt_dm_root.py index edee3851866..f08ba30dec8 100644 --- a/tests/gateway/relay/test_relay_slack_prompt_dm_root.py +++ b/tests/gateway/relay/test_relay_slack_prompt_dm_root.py @@ -1,21 +1,24 @@ -"""Slack relay: interactive prompts (approval / clarify) must post FLAT at the DM root. +"""Slack relay: interactive prompts follow the turn's thread stamp (QA-5). -Reported symptom (live): an approval / clarify Block Kit card raised mid-turn in -a Slack DM was posted THREADED under the triggering message instead of at the DM -root ("the approval block was put in a thread and did not follow the setting"). +The threading MODE (flat DM vs thread-per-message) is decided in exactly ONE +place: run.py's ``_resolve_progress_thread_id``, which reads +``platforms.slack.extra.reply_in_thread`` and encodes the verdict into the +outbound ``metadata`` stamp: -Root cause: a clarify/approval prompt is emitted in reply to the triggering -inbound event, so the metadata handed to ``_send_prompt`` carries that event's -thread context — run.py's ``_thread_metadata_for_source`` stamps -``metadata["thread_id"]`` (for a Slack DM, the triggering message's own ts, used -only as a session-keying fallback), plus ``metadata["message_id"]`` = that same -ts. Forwarding ``thread_id`` makes the connector's slackRestSender thread the -prompt card UNDER the user's message. Native Slack Hermes suppresses this -synthetic DM thread anchor (``SlackAdapter._resolve_thread_ts``); the relay lane -had no such disambiguation. + * flat mode -> the synthetic self-anchor is suppressed in run.py, so prompt + metadata arrives with NO ``thread_id`` and the card posts at the DM root; + * thread-per-message (default) -> ``metadata.thread_id`` is stamped for the + whole turn; on the FIRST turn it legitimately equals the triggering + message's ts (the synthetic root IS the thread). -These are behaviour-contract tests: they assert how the outbound ``prompt`` frame -relates to the chat type + inherited thread metadata (the invariant the connector +The prompt lane must TRUST that stamp, like ``_resolve_reply_to_for_send`` +does. Re-deriving the mode here (the old unconditional +``thread_id == message_id`` strip) exiled the approval card and its +resolved-state swap to the DM root while progress bubbles honoured the thread +(the 2026-07-27 mixed-placement report). + +These are behaviour-contract tests: they assert how the outbound ``prompt`` +frame relates to the inherited thread metadata (the invariant the connector depends on), not a snapshot. They drive the REAL ``RelayAdapter`` + ``StubConnector`` end to end. """ @@ -83,15 +86,39 @@ def _last_prompt(stub) -> dict: # --------------------------------------------------------------------------- -# DM-root: the synthetic self-anchor is stripped +# Flat mode: run.py stamps NO thread_id -> the card posts at the DM root. # --------------------------------------------------------------------------- @pytest.mark.asyncio -async def test_exec_approval_posts_flat_at_dm_root(): - """A Slack DM approval prompt must NOT inherit the triggering message's - synthetic thread_id — it posts flat at the DM root, matching native.""" +async def test_exec_approval_flat_mode_posts_at_dm_root(): + """Flat-DM turn (reply_in_thread=false): run.py suppressed the synthetic + anchor upstream, so prompt metadata has no thread_id and none appears on + the wire — the card posts at the DM root.""" + adapter, stub = _wire("D1", "dm", scope_id="T1") + md = {"message_id": "1700000000.000100", "scope_id": "T1"} + result = await adapter.send_exec_approval( + "D1", "rm -rf /tmp/x", "sess:1", description="deletes files", metadata=md + ) + assert result.success is True + frame = _last_prompt(stub) + meta = frame["metadata"] or {} + assert "thread_id" not in meta + assert "thread_ts" not in meta + # reply_to on the outbound action stays unset — a root-level post. + assert frame["reply_to"] is None + # Tenant scope is preserved untouched (egress routing must not break). + assert meta.get("scope_id") == "T1" + + +# --------------------------------------------------------------------------- +# Thread-per-message mode: the first-turn self-anchor (thread_id == message_id) +# IS the thread root — the prompt must stay in the thread (QA-5 regression). +# --------------------------------------------------------------------------- +@pytest.mark.asyncio +async def test_exec_approval_first_turn_self_anchor_stays_in_thread(): + """Thread-per-message first turn: run.py stamps thread_id = the triggering + message's own ts. The approval card must post INTO that thread — stripping + it exiled the card to the home channel (2026-07-27 report).""" adapter, stub = _wire("D1", "dm", scope_id="T1") - # run.py hands the prompt the triggering message's thread context: for a DM - # with no real thread, thread_id == message_id (the synthetic self-anchor). md = { "thread_id": "1700000000.000100", "message_id": "1700000000.000100", @@ -103,22 +130,14 @@ async def test_exec_approval_posts_flat_at_dm_root(): assert result.success is True frame = _last_prompt(stub) meta = frame["metadata"] or {} - # The inherited synthetic thread anchor is dropped so it posts at the DM root. - assert "thread_id" not in meta, ( - "approval prompt must NOT inherit the triggering message thread_id" + assert meta.get("thread_id") == "1700000000.000100", ( + "first-turn self-anchor is the thread root; the prompt must honour it" ) - assert "thread_ts" not in meta - # reply_to on the outbound action stays unset — a root-level post. - assert frame["reply_to"] is None - # Tenant scope is preserved untouched (egress routing must not break). assert meta.get("scope_id") == "T1" - # The caller's original metadata dict was not mutated in place. - assert md.get("thread_id") == "1700000000.000100" @pytest.mark.asyncio -async def test_clarify_posts_flat_at_dm_root(): - """A Slack DM clarify prompt (with choices) also posts flat at the DM root.""" +async def test_clarify_first_turn_self_anchor_stays_in_thread(): adapter, stub = _wire("D1", "dm", scope_id="T1") md = { "thread_id": "1700000000.000200", @@ -131,22 +150,21 @@ async def test_clarify_posts_flat_at_dm_root(): assert result.success is True frame = _last_prompt(stub) meta = frame["metadata"] or {} - assert "thread_id" not in meta - assert "thread_ts" not in meta - assert frame["reply_to"] is None + assert meta.get("thread_id") == "1700000000.000200" assert meta.get("scope_id") == "T1" @pytest.mark.asyncio -async def test_slash_confirm_posts_flat_at_dm_root(): - """The DM-root rule covers every prompt surface (single _send_prompt choke).""" +async def test_slash_confirm_first_turn_self_anchor_stays_in_thread(): + """The stamp-trusting rule covers every prompt surface (single + _send_prompt choke point).""" adapter, stub = _wire("D1", "dm") md = {"thread_id": "1700000000.000300", "message_id": "1700000000.000300"} await adapter.send_slash_confirm( "D1", "Reload MCP", "invalidates cache", "s", "cf-1", metadata=md ) frame = _last_prompt(stub) - assert "thread_id" not in (frame["metadata"] or {}) + assert (frame["metadata"] or {}).get("thread_id") == "1700000000.000300" # --------------------------------------------------------------------------- @@ -155,8 +173,7 @@ async def test_slash_confirm_posts_flat_at_dm_root(): @pytest.mark.asyncio async def test_exec_approval_in_real_thread_keeps_thread_id(): """A DM prompt raised inside a REAL thread (thread_id distinct from the - triggering message ts) must STAY in that thread — only the synthetic - self-anchor is stripped.""" + triggering message ts) stays in that thread.""" adapter, stub = _wire("D1", "dm", scope_id="T1") md = { "thread_id": "1699000000.999000", @@ -170,8 +187,7 @@ async def test_exec_approval_in_real_thread_keeps_thread_id(): @pytest.mark.asyncio async def test_channel_approval_keeps_thread_id(): - """A Slack CHANNEL prompt keeps its thread_id (autoThread / real thread); - the DM-only guard must not touch a non-DM chat.""" + """A Slack CHANNEL prompt keeps its thread_id (autoThread / real thread).""" adapter, stub = _wire("C1", "channel", scope_id="T1") md = { "thread_id": "1700000000.000400", @@ -185,8 +201,8 @@ async def test_channel_approval_keeps_thread_id(): @pytest.mark.asyncio async def test_non_slack_dm_approval_keeps_thread_id(): - """The disambiguation is Slack-scoped: a non-Slack relay DM keeps thread_id - (its connector owns its own threading semantics).""" + """A non-Slack relay DM keeps thread_id (its connector owns its own + threading semantics).""" adapter, stub = _wire("dc1", "dm", platform=Platform.DISCORD) md = {"thread_id": "9000", "message_id": "9000"} await adapter.send_exec_approval("dc1", "cmd", "s", metadata=md)