From 12ab0ce062ec944be97ff0a67b9d22a25f70ac71 Mon Sep 17 00:00:00 2001 From: Spencer Murray Date: Thu, 25 Jun 2026 11:19:17 -0400 Subject: [PATCH 1/5] Add Slack handler to reset manual incident-channel topic changes --- slack-app-manifest.yaml | 4 + src/firetower/slack_app/bolt.py | 9 ++ .../slack_app/handlers/topic_guard.py | 69 ++++++++++++++ .../slack_app/tests/test_topic_guard.py | 89 +++++++++++++++++++ 4 files changed, 171 insertions(+) create mode 100644 src/firetower/slack_app/handlers/topic_guard.py create mode 100644 src/firetower/slack_app/tests/test_topic_guard.py diff --git a/slack-app-manifest.yaml b/slack-app-manifest.yaml index 40ecb8dd..67427d8a 100644 --- a/slack-app-manifest.yaml +++ b/slack-app-manifest.yaml @@ -33,6 +33,10 @@ oauth_config: - users:read.email pkce_enabled: false settings: + event_subscriptions: + bot_events: + - message.channels + - message.groups interactivity: is_enabled: true org_deploy_enabled: false diff --git a/src/firetower/slack_app/bolt.py b/src/firetower/slack_app/bolt.py index b0c9c699..0516f8ba 100644 --- a/src/firetower/slack_app/bolt.py +++ b/src/firetower/slack_app/bolt.py @@ -47,6 +47,7 @@ handle_statuspage_submission, ) from firetower.slack_app.handlers.subject import handle_subject_command +from firetower.slack_app.handlers.topic_guard import handle_channel_topic_change from firetower.slack_app.handlers.update_incident import ( handle_update_command, handle_update_incident_submission, @@ -108,6 +109,7 @@ def get_bolt_app() -> App: _bolt_app.command("/inc")(handle_command) _bolt_app.command("/testinc")(handle_command) _register_views(_bolt_app) + _register_event_handlers(_bolt_app) return _bolt_app @@ -256,3 +258,10 @@ def _register_views(app: App) -> None: "affected_region_tags", ): app.options(action_id)(handle_tag_options) + + +def _register_event_handlers(app: App) -> None: + """Register Slack event subscriptions on the Bolt app.""" + app.event({"type": "message", "subtype": "channel_topic"})( + _with_metrics("channel_topic")(handle_channel_topic_change) + ) diff --git a/src/firetower/slack_app/handlers/topic_guard.py b/src/firetower/slack_app/handlers/topic_guard.py new file mode 100644 index 00000000..ab1ac467 --- /dev/null +++ b/src/firetower/slack_app/handlers/topic_guard.py @@ -0,0 +1,69 @@ +import logging +from typing import Any + +from firetower.incidents.hooks import _slack_service, build_channel_topic +from firetower.slack_app.handlers.utils import get_incident_from_channel + +logger = logging.getLogger(__name__) + +_bot_user_id: str | None = None + + +def _get_bot_user_id(client: Any) -> str | None: + """Return the bot's own Slack user id, caching it after the first lookup.""" + global _bot_user_id # noqa: PLW0603 + if _bot_user_id is None: + _bot_user_id = client.auth_test()["user_id"] + return _bot_user_id + + +def _build_reset_message(attempted: str) -> str: + lines = [ + "Channel topics here are managed by Firetower and reflect the incident's " + "current status, so I reset it.", + ] + if attempted: + quoted = "\n".join(f"> {line}" for line in attempted.splitlines()) + lines.append(f"You tried to set:\n{quoted}") + lines.append( + "To change incident details (which update the topic automatically), use " + "`/ft subject `, `/ft severity <P0-P4>`, or `/ft captain @user`." + ) + return "\n\n".join(lines) + + +def handle_channel_topic_change(event: dict, client: Any, logger: Any = logger) -> None: + """Reset manual incident-channel topic edits and nudge the editor. + + Slack emits a ``channel_topic`` message event whenever a channel's topic + changes, including when Firetower sets it. The bot-author guard prevents an + infinite reset loop on our own edits. + """ + if event.get("user") == _get_bot_user_id(client): + return + + channel_id = event["channel"] + incident = get_incident_from_channel(channel_id) + if incident is None: + return + + canonical = build_channel_topic(incident) + attempted = event.get("topic", "") + if attempted == canonical: + return + + logger.info( + "Resetting manual topic change in incident channel %s (incident %s)", + channel_id, + incident.incident_number, + ) + _slack_service.set_channel_topic(channel_id, canonical) + + try: + client.chat_postEphemeral( + channel=channel_id, + user=event.get("user"), + text=_build_reset_message(attempted), + ) + except Exception: + logger.exception("Failed to post topic-reset ephemeral to %s", channel_id) diff --git a/src/firetower/slack_app/tests/test_topic_guard.py b/src/firetower/slack_app/tests/test_topic_guard.py new file mode 100644 index 00000000..b57495ce --- /dev/null +++ b/src/firetower/slack_app/tests/test_topic_guard.py @@ -0,0 +1,89 @@ +from unittest.mock import MagicMock, patch + +import pytest + +from firetower.slack_app.handlers import topic_guard +from firetower.slack_app.handlers.topic_guard import handle_channel_topic_change + +CHANNEL_ID = "C_TEST_CHANNEL" +BOT_USER_ID = "U0000" +CANONICAL = "[P2] <https://ft/INC-1|INC-1 Test>" + + +@pytest.fixture(autouse=True) +def reset_bot_user_cache(): + topic_guard._bot_user_id = None + yield + topic_guard._bot_user_id = None + + +@pytest.fixture +def client(): + c = MagicMock() + c.auth_test.return_value = {"user_id": BOT_USER_ID} + return c + + +def _event(user="U_OTHER", topic="Something custom", channel=CHANNEL_ID): + return { + "type": "message", + "subtype": "channel_topic", + "channel": channel, + "user": user, + "topic": topic, + } + + +@patch("firetower.slack_app.handlers.topic_guard._slack_service") +@patch("firetower.slack_app.handlers.topic_guard.build_channel_topic") +@patch("firetower.slack_app.handlers.topic_guard.get_incident_from_channel") +def test_manual_change_resets_and_notifies( + mock_get_incident, mock_build_topic, mock_slack_service, client +): + mock_get_incident.return_value = MagicMock() + mock_build_topic.return_value = CANONICAL + + handle_channel_topic_change(_event(topic="My custom topic"), client) + + mock_slack_service.set_channel_topic.assert_called_once_with(CHANNEL_ID, CANONICAL) + client.chat_postEphemeral.assert_called_once() + kwargs = client.chat_postEphemeral.call_args[1] + assert kwargs["channel"] == CHANNEL_ID + assert kwargs["user"] == "U_OTHER" + assert "My custom topic" in kwargs["text"] + + +@patch("firetower.slack_app.handlers.topic_guard._slack_service") +@patch("firetower.slack_app.handlers.topic_guard.get_incident_from_channel") +def test_bot_authored_event_is_ignored(mock_get_incident, mock_slack_service, client): + handle_channel_topic_change(_event(user=BOT_USER_ID), client) + + mock_get_incident.assert_not_called() + mock_slack_service.set_channel_topic.assert_not_called() + client.chat_postEphemeral.assert_not_called() + + +@patch("firetower.slack_app.handlers.topic_guard._slack_service") +@patch("firetower.slack_app.handlers.topic_guard.get_incident_from_channel") +def test_non_incident_channel_is_noop(mock_get_incident, mock_slack_service, client): + mock_get_incident.return_value = None + + handle_channel_topic_change(_event(), client) + + mock_slack_service.set_channel_topic.assert_not_called() + client.chat_postEphemeral.assert_not_called() + + +@patch("firetower.slack_app.handlers.topic_guard._slack_service") +@patch("firetower.slack_app.handlers.topic_guard.build_channel_topic") +@patch("firetower.slack_app.handlers.topic_guard.get_incident_from_channel") +def test_topic_already_canonical_is_noop( + mock_get_incident, mock_build_topic, mock_slack_service, client +): + mock_get_incident.return_value = MagicMock() + mock_build_topic.return_value = CANONICAL + + handle_channel_topic_change(_event(topic=CANONICAL), client) + + mock_slack_service.set_channel_topic.assert_not_called() + client.chat_postEphemeral.assert_not_called() From 7773d0e765bceab5554dc81744dbfda3ab1788de Mon Sep 17 00:00:00 2001 From: Spencer Murray <spencer.murray@sentry.io> Date: Thu, 25 Jun 2026 11:22:23 -0400 Subject: [PATCH 2/5] Escape user-supplied topic text in topic-reset ephemeral --- .../slack_app/handlers/topic_guard.py | 5 ++++- .../slack_app/tests/test_topic_guard.py | 21 +++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/firetower/slack_app/handlers/topic_guard.py b/src/firetower/slack_app/handlers/topic_guard.py index ab1ac467..8b107720 100644 --- a/src/firetower/slack_app/handlers/topic_guard.py +++ b/src/firetower/slack_app/handlers/topic_guard.py @@ -2,6 +2,7 @@ from typing import Any from firetower.incidents.hooks import _slack_service, build_channel_topic +from firetower.integrations.services.slack import escape_slack_text from firetower.slack_app.handlers.utils import get_incident_from_channel logger = logging.getLogger(__name__) @@ -23,7 +24,9 @@ def _build_reset_message(attempted: str) -> str: "current status, so I reset it.", ] if attempted: - quoted = "\n".join(f"> {line}" for line in attempted.splitlines()) + quoted = "\n".join( + f"> {escape_slack_text(line)}" for line in attempted.splitlines() + ) lines.append(f"You tried to set:\n{quoted}") lines.append( "To change incident details (which update the topic automatically), use " diff --git a/src/firetower/slack_app/tests/test_topic_guard.py b/src/firetower/slack_app/tests/test_topic_guard.py index b57495ce..44c316bc 100644 --- a/src/firetower/slack_app/tests/test_topic_guard.py +++ b/src/firetower/slack_app/tests/test_topic_guard.py @@ -74,6 +74,27 @@ def test_non_incident_channel_is_noop(mock_get_incident, mock_slack_service, cli client.chat_postEphemeral.assert_not_called() +@patch("firetower.slack_app.handlers.topic_guard._slack_service") +@patch("firetower.slack_app.handlers.topic_guard.build_channel_topic") +@patch("firetower.slack_app.handlers.topic_guard.get_incident_from_channel") +def test_attempted_topic_is_escaped( + mock_get_incident, mock_build_topic, mock_slack_service, client +): + mock_get_incident.return_value = MagicMock() + mock_build_topic.return_value = CANONICAL + + handle_channel_topic_change( + _event(topic="<!channel> see <https://evil|here> & <@U999>"), client + ) + + text = client.chat_postEphemeral.call_args[1]["text"] + assert "<!channel>" not in text + assert "<@U999>" not in text + assert "<https://evil|here>" not in text + assert "<!channel>" in text + assert "&" in text + + @patch("firetower.slack_app.handlers.topic_guard._slack_service") @patch("firetower.slack_app.handlers.topic_guard.build_channel_topic") @patch("firetower.slack_app.handlers.topic_guard.get_incident_from_channel") From d1ea60fbebc9051845601fecd71822c74ffe3ae2 Mon Sep 17 00:00:00 2001 From: Spencer Murray <spencer.murray@sentry.io> Date: Thu, 25 Jun 2026 12:29:46 -0400 Subject: [PATCH 3/5] Harden topic guard with firehose no-op, Slack retry dedup, and thread-safe cache --- src/firetower/slack_app/bolt.py | 42 ++++++++- .../slack_app/handlers/topic_guard.py | 74 +++++++++++++-- .../slack_app/tests/test_topic_guard.py | 92 ++++++++++++++++++- 3 files changed, 195 insertions(+), 13 deletions(-) diff --git a/src/firetower/slack_app/bolt.py b/src/firetower/slack_app/bolt.py index 0516f8ba..d9466876 100644 --- a/src/firetower/slack_app/bolt.py +++ b/src/firetower/slack_app/bolt.py @@ -215,6 +215,33 @@ def wrapper(*args: Any, **kwargs: Any) -> Any: return decorator +def _with_event_metrics(event_type: str) -> Callable[..., Callable[..., Any]]: + """Wrap a Bolt event handler to emit submitted/completed/failed metrics. + + Uses a ``slack_app.events`` namespace so event volume does not pollute the + ``slack_app.views`` dashboards. + """ + + def decorator(func: Callable[..., Any]) -> Callable[..., Any]: + @wraps(func) + def wrapper(*args: Any, **kwargs: Any) -> Any: + close_old_connections() + tags = [f"event_type:{event_type}"] + statsd.increment("slack_app.events.submitted", tags=tags) + try: + result = func(*args, **kwargs) + statsd.increment("slack_app.events.completed", tags=tags) + return result + except Exception: + logger.exception("Event handler failed: %s", event_type) + statsd.increment("slack_app.events.failed", tags=tags) + raise + + return wrapper + + return decorator + + def _register_views(app: App) -> None: """Register view handlers (modals, etc.) on the Bolt app.""" app.view("backfill_incident_modal")( @@ -260,8 +287,21 @@ def _register_views(app: App) -> None: app.options(action_id)(handle_tag_options) +def _ignore_message_event(ack: Any) -> None: + """No-op catch-all for message events. + + Subscribing to message.channels/message.groups opts the bot into the full + message firehose. Without a catch-all listener Bolt logs an "Unhandled + request" warning for every non-topic message. This listener is registered + after the channel_topic handler, so for topic changes the specific handler + runs first and returns before this one is reached. + """ + ack() + + def _register_event_handlers(app: App) -> None: """Register Slack event subscriptions on the Bolt app.""" app.event({"type": "message", "subtype": "channel_topic"})( - _with_metrics("channel_topic")(handle_channel_topic_change) + _with_event_metrics("channel_topic")(handle_channel_topic_change) ) + app.event("message")(_ignore_message_event) diff --git a/src/firetower/slack_app/handlers/topic_guard.py b/src/firetower/slack_app/handlers/topic_guard.py index 8b107720..004e391a 100644 --- a/src/firetower/slack_app/handlers/topic_guard.py +++ b/src/firetower/slack_app/handlers/topic_guard.py @@ -1,23 +1,74 @@ import logging +import threading +from collections import OrderedDict from typing import Any -from firetower.incidents.hooks import _slack_service, build_channel_topic -from firetower.integrations.services.slack import escape_slack_text +from firetower.incidents.hooks import build_channel_topic +from firetower.integrations.services.slack import SlackService, escape_slack_text from firetower.slack_app.handlers.utils import get_incident_from_channel logger = logging.getLogger(__name__) +_slack_service = SlackService() + _bot_user_id: str | None = None +# Bounded set of recently handled channel_topic event ids, used to drop Slack +# redeliveries. Socket Mode strips the X-Slack-Retry-Num header (it lives on the +# envelope, not the event payload Bolt forwards), so we dedup on the event's own +# id/timestamp instead. +_RECENT_EVENT_CACHE_SIZE = 256 +_recent_event_ids: "OrderedDict[str, None]" = OrderedDict() +# Bolt dispatches event listeners on a thread pool, so guard the cache against +# concurrent check-then-set/evict races. +_recent_event_ids_lock = threading.Lock() + def _get_bot_user_id(client: Any) -> str | None: """Return the bot's own Slack user id, caching it after the first lookup.""" global _bot_user_id # noqa: PLW0603 if _bot_user_id is None: - _bot_user_id = client.auth_test()["user_id"] + try: + _bot_user_id = client.auth_test()["user_id"] + except Exception: + logger.exception("auth_test failed while resolving bot user id") + return None return _bot_user_id +def _event_id(event: dict) -> str | None: + return event.get("event_ts") or event.get("ts") + + +def _seen_recently(event_id: str) -> bool: + """Return True if this event id was already handled, recording it otherwise.""" + with _recent_event_ids_lock: + if event_id in _recent_event_ids: + return True + _recent_event_ids[event_id] = None + while len(_recent_event_ids) > _RECENT_EVENT_CACHE_SIZE: + _recent_event_ids.popitem(last=False) + return False + + +def _is_slack_retry(request: Any) -> bool: + """Best-effort detection of a Slack event redelivery. + + Works in HTTP mode where Bolt exposes the X-Slack-Retry-Num header. Under + Socket Mode (how Firetower runs) the header is absent, so callers must also + rely on the event-id dedup cache. + """ + if request is None: + return False + headers = getattr(request, "headers", None) + if not headers: + return False + value = headers.get("x-slack-retry-num") + if isinstance(value, (list, tuple)): + value = value[0] if value else None + return bool(value) + + def _build_reset_message(attempted: str) -> str: lines = [ "Channel topics here are managed by Firetower and reflect the incident's " @@ -35,25 +86,32 @@ def _build_reset_message(attempted: str) -> str: return "\n\n".join(lines) -def handle_channel_topic_change(event: dict, client: Any, logger: Any = logger) -> None: +def handle_channel_topic_change(event: dict, client: Any, request: Any = None) -> None: """Reset manual incident-channel topic edits and nudge the editor. Slack emits a ``channel_topic`` message event whenever a channel's topic changes, including when Firetower sets it. The bot-author guard prevents an - infinite reset loop on our own edits. + infinite reset loop on our own edits, and a retry/dedup guard prevents a + second reset and ephemeral when Slack redelivers the event. """ if event.get("user") == _get_bot_user_id(client): return - channel_id = event["channel"] + channel_id = event.get("channel") + if not channel_id: + return + + event_id = _event_id(event) + if _is_slack_retry(request) or (event_id is not None and _seen_recently(event_id)): + logger.info("Skipping duplicate channel_topic event for channel %s", channel_id) + return + incident = get_incident_from_channel(channel_id) if incident is None: return canonical = build_channel_topic(incident) attempted = event.get("topic", "") - if attempted == canonical: - return logger.info( "Resetting manual topic change in incident channel %s (incident %s)", diff --git a/src/firetower/slack_app/tests/test_topic_guard.py b/src/firetower/slack_app/tests/test_topic_guard.py index 44c316bc..6b4cab8f 100644 --- a/src/firetower/slack_app/tests/test_topic_guard.py +++ b/src/firetower/slack_app/tests/test_topic_guard.py @@ -11,10 +11,12 @@ @pytest.fixture(autouse=True) -def reset_bot_user_cache(): +def reset_module_state(): topic_guard._bot_user_id = None + topic_guard._recent_event_ids.clear() yield topic_guard._bot_user_id = None + topic_guard._recent_event_ids.clear() @pytest.fixture @@ -24,13 +26,20 @@ def client(): return c -def _event(user="U_OTHER", topic="Something custom", channel=CHANNEL_ID): +_TS_COUNTER = [0] + + +def _event(user="U_OTHER", topic="Something custom", channel=CHANNEL_ID, ts=None): + if ts is None: + _TS_COUNTER[0] += 1 + ts = f"1700000000.{_TS_COUNTER[0]:06d}" return { "type": "message", "subtype": "channel_topic", "channel": channel, "user": user, "topic": topic, + "event_ts": ts, } @@ -74,6 +83,19 @@ def test_non_incident_channel_is_noop(mock_get_incident, mock_slack_service, cli client.chat_postEphemeral.assert_not_called() +@patch("firetower.slack_app.handlers.topic_guard._slack_service") +@patch("firetower.slack_app.handlers.topic_guard.get_incident_from_channel") +def test_missing_channel_is_noop(mock_get_incident, mock_slack_service, client): + event = _event() + del event["channel"] + + handle_channel_topic_change(event, client) + + mock_get_incident.assert_not_called() + mock_slack_service.set_channel_topic.assert_not_called() + client.chat_postEphemeral.assert_not_called() + + @patch("firetower.slack_app.handlers.topic_guard._slack_service") @patch("firetower.slack_app.handlers.topic_guard.build_channel_topic") @patch("firetower.slack_app.handlers.topic_guard.get_incident_from_channel") @@ -98,13 +120,75 @@ def test_attempted_topic_is_escaped( @patch("firetower.slack_app.handlers.topic_guard._slack_service") @patch("firetower.slack_app.handlers.topic_guard.build_channel_topic") @patch("firetower.slack_app.handlers.topic_guard.get_incident_from_channel") -def test_topic_already_canonical_is_noop( +def test_ephemeral_failure_still_resets( + mock_get_incident, mock_build_topic, mock_slack_service, client +): + mock_get_incident.return_value = MagicMock() + mock_build_topic.return_value = CANONICAL + client.chat_postEphemeral.side_effect = Exception("boom") + + handle_channel_topic_change(_event(), client) + + mock_slack_service.set_channel_topic.assert_called_once_with(CHANNEL_ID, CANONICAL) + client.chat_postEphemeral.assert_called_once() + + +@patch("firetower.slack_app.handlers.topic_guard._slack_service") +@patch("firetower.slack_app.handlers.topic_guard.build_channel_topic") +@patch("firetower.slack_app.handlers.topic_guard.get_incident_from_channel") +def test_bot_id_cache_is_reused( + mock_get_incident, mock_build_topic, mock_slack_service, client +): + mock_get_incident.return_value = MagicMock() + mock_build_topic.return_value = CANONICAL + + handle_channel_topic_change(_event(), client) + handle_channel_topic_change(_event(), client) + + assert client.auth_test.call_count == 1 + + +@patch("firetower.slack_app.handlers.topic_guard._slack_service") +@patch("firetower.slack_app.handlers.topic_guard.get_incident_from_channel") +def test_auth_test_failure_does_not_crash( + mock_get_incident, mock_slack_service, client +): + client.auth_test.side_effect = Exception("transient") + + handle_channel_topic_change(_event(), client) + + mock_get_incident.assert_called_once() + + +@patch("firetower.slack_app.handlers.topic_guard._slack_service") +@patch("firetower.slack_app.handlers.topic_guard.build_channel_topic") +@patch("firetower.slack_app.handlers.topic_guard.get_incident_from_channel") +def test_duplicate_event_id_is_skipped( + mock_get_incident, mock_build_topic, mock_slack_service, client +): + mock_get_incident.return_value = MagicMock() + mock_build_topic.return_value = CANONICAL + event = _event(ts="1700000000.999999") + + handle_channel_topic_change(event, client) + handle_channel_topic_change(event, client) + + mock_slack_service.set_channel_topic.assert_called_once() + client.chat_postEphemeral.assert_called_once() + + +@patch("firetower.slack_app.handlers.topic_guard._slack_service") +@patch("firetower.slack_app.handlers.topic_guard.build_channel_topic") +@patch("firetower.slack_app.handlers.topic_guard.get_incident_from_channel") +def test_slack_retry_header_is_skipped( mock_get_incident, mock_build_topic, mock_slack_service, client ): mock_get_incident.return_value = MagicMock() mock_build_topic.return_value = CANONICAL + request = MagicMock() + request.headers = {"x-slack-retry-num": ["1"]} - handle_channel_topic_change(_event(topic=CANONICAL), client) + handle_channel_topic_change(_event(), client, request=request) mock_slack_service.set_channel_topic.assert_not_called() client.chat_postEphemeral.assert_not_called() From 2c27ac3b9a5c5baeea18eebe7b63d94350669a88 Mon Sep 17 00:00:00 2001 From: Spencer Murray <spencer.murray@sentry.io> Date: Thu, 25 Jun 2026 12:40:35 -0400 Subject: [PATCH 4/5] Drop dead Socket Mode retry-header check from topic guard --- .../slack_app/handlers/topic_guard.py | 28 ++++--------------- .../slack_app/tests/test_topic_guard.py | 17 ----------- 2 files changed, 6 insertions(+), 39 deletions(-) diff --git a/src/firetower/slack_app/handlers/topic_guard.py b/src/firetower/slack_app/handlers/topic_guard.py index 004e391a..e11d9e9e 100644 --- a/src/firetower/slack_app/handlers/topic_guard.py +++ b/src/firetower/slack_app/handlers/topic_guard.py @@ -16,7 +16,9 @@ # Bounded set of recently handled channel_topic event ids, used to drop Slack # redeliveries. Socket Mode strips the X-Slack-Retry-Num header (it lives on the # envelope, not the event payload Bolt forwards), so we dedup on the event's own -# id/timestamp instead. +# id/timestamp instead. This in-memory cache is per-process, so it only dedups +# correctly while the firetower-slack-app Cloud Run service stays single-instance +# (max_instance_count = 1); scaling it out would split the cache across replicas. _RECENT_EVENT_CACHE_SIZE = 256 _recent_event_ids: "OrderedDict[str, None]" = OrderedDict() # Bolt dispatches event listeners on a thread pool, so guard the cache against @@ -51,24 +53,6 @@ def _seen_recently(event_id: str) -> bool: return False -def _is_slack_retry(request: Any) -> bool: - """Best-effort detection of a Slack event redelivery. - - Works in HTTP mode where Bolt exposes the X-Slack-Retry-Num header. Under - Socket Mode (how Firetower runs) the header is absent, so callers must also - rely on the event-id dedup cache. - """ - if request is None: - return False - headers = getattr(request, "headers", None) - if not headers: - return False - value = headers.get("x-slack-retry-num") - if isinstance(value, (list, tuple)): - value = value[0] if value else None - return bool(value) - - def _build_reset_message(attempted: str) -> str: lines = [ "Channel topics here are managed by Firetower and reflect the incident's " @@ -86,12 +70,12 @@ def _build_reset_message(attempted: str) -> str: return "\n\n".join(lines) -def handle_channel_topic_change(event: dict, client: Any, request: Any = None) -> None: +def handle_channel_topic_change(event: dict, client: Any) -> None: """Reset manual incident-channel topic edits and nudge the editor. Slack emits a ``channel_topic`` message event whenever a channel's topic changes, including when Firetower sets it. The bot-author guard prevents an - infinite reset loop on our own edits, and a retry/dedup guard prevents a + infinite reset loop on our own edits, and the event-id dedup guard prevents a second reset and ephemeral when Slack redelivers the event. """ if event.get("user") == _get_bot_user_id(client): @@ -102,7 +86,7 @@ def handle_channel_topic_change(event: dict, client: Any, request: Any = None) - return event_id = _event_id(event) - if _is_slack_retry(request) or (event_id is not None and _seen_recently(event_id)): + if event_id is not None and _seen_recently(event_id): logger.info("Skipping duplicate channel_topic event for channel %s", channel_id) return diff --git a/src/firetower/slack_app/tests/test_topic_guard.py b/src/firetower/slack_app/tests/test_topic_guard.py index 6b4cab8f..b752a2dc 100644 --- a/src/firetower/slack_app/tests/test_topic_guard.py +++ b/src/firetower/slack_app/tests/test_topic_guard.py @@ -175,20 +175,3 @@ def test_duplicate_event_id_is_skipped( mock_slack_service.set_channel_topic.assert_called_once() client.chat_postEphemeral.assert_called_once() - - -@patch("firetower.slack_app.handlers.topic_guard._slack_service") -@patch("firetower.slack_app.handlers.topic_guard.build_channel_topic") -@patch("firetower.slack_app.handlers.topic_guard.get_incident_from_channel") -def test_slack_retry_header_is_skipped( - mock_get_incident, mock_build_topic, mock_slack_service, client -): - mock_get_incident.return_value = MagicMock() - mock_build_topic.return_value = CANONICAL - request = MagicMock() - request.headers = {"x-slack-retry-num": ["1"]} - - handle_channel_topic_change(_event(), client, request=request) - - mock_slack_service.set_channel_topic.assert_not_called() - client.chat_postEphemeral.assert_not_called() From eadf6477d1605912ea0756e9ab780be6b39b72a6 Mon Sep 17 00:00:00 2001 From: Spencer Murray <spencer.murray@sentry.io> Date: Thu, 25 Jun 2026 12:44:58 -0400 Subject: [PATCH 5/5] Fix /ft captain usage hint in topic-reset ephemeral --- src/firetower/slack_app/handlers/topic_guard.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/firetower/slack_app/handlers/topic_guard.py b/src/firetower/slack_app/handlers/topic_guard.py index e11d9e9e..b1533250 100644 --- a/src/firetower/slack_app/handlers/topic_guard.py +++ b/src/firetower/slack_app/handlers/topic_guard.py @@ -65,7 +65,7 @@ def _build_reset_message(attempted: str) -> str: lines.append(f"You tried to set:\n{quoted}") lines.append( "To change incident details (which update the topic automatically), use " - "`/ft subject <title>`, `/ft severity <P0-P4>`, or `/ft captain @user`." + "`/ft subject <title>`, `/ft severity <P0-P4>`, or `/ft captain`." ) return "\n\n".join(lines)