Skip to content

fix(slack): prevent thread parent edit replays - #73450

Open
nahyeongjin1 wants to merge 2 commits into
NousResearch:mainfrom
nahyeongjin1:fix/slack-thread-parent-replay
Open

fix(slack): prevent thread parent edit replays#73450
nahyeongjin1 wants to merge 2 commits into
NousResearch:mainfrom
nahyeongjin1:fix/slack-thread-parent-replay

Conversation

@nahyeongjin1

@nahyeongjin1 nahyeongjin1 commented Jul 28, 2026

Copy link
Copy Markdown

What does this PR do?

Slack can emit message_changed for an existing thread parent. In the incident that motivated this follow-up, the Slack thread contained a root text edit but no new user reply; the adapter normalized that edit as fresh inbound input when the root timestamp was absent from the process-local _processed_message_ts set.

A missing in-memory timestamp does not prove that the root was never delivered. Replaying an already-addressed parent can therefore create a phantom user turn from instructions the agent already acted on.

This PR now handles both replay classes before message normalization:

  • filters metadata-only parent updates without suppressing proven visible text, block, attachment, or file additions/removals
  • detects replied parents whose pre-edit snapshot already addressed this workspace's bot by mention, Block Kit, or configured wake pattern
  • suppresses later edits of those addressed parents even if the mention is removed or the current payload is malformed
  • routes a newly added mention exactly once only when the previous snapshot reliably proves that the bot was not addressed
  • fails closed when the current edit addresses the bot but previous_message is missing, partial, or malformed
  • preserves existing visible-edit routing when there is no bot-address evidence, and preserves edits with no reply evidence
  • validates reply evidence and traversed Block Kit shapes before using them as proof
  • resolves one workspace-local bot identity, with the edited message's team used only as a fallback when the outer event has none

Users who want revised parent text acted on can send a new thread reply. The adapter does not synthesize one and does not add durable session infrastructure.

Related Issue

N/A — reproduced from a live Slack Socket Mode event sequence and covered by regression tests.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • plugins/platforms/slack/adapter.py
    • classifies visible parent edits before normalization
    • validates thread-reply evidence
    • derives a tri-state pre-edit bot-address result from trusted text and Block Kit data
    • blocks answered-parent replays while preserving proven absent-to-present mention transitions
    • keeps workspace bot identity scoped to the event team
  • tests/gateway/test_slack.py
    • covers cold process state, repeated edits, mention and wake-word removal, malformed current and previous payloads, nested Block Kit corruption, valid Block Kit transitions, malformed/valid reply evidence, multi-workspace identity, and preserved visible-edit paths

How to Test

scripts/run_tests.sh tests/gateway/test_slack.py -q
ruff check plugins/platforms/slack/adapter.py tests/gateway/test_slack.py
python -m py_compile plugins/platforms/slack/adapter.py tests/gateway/test_slack.py
git diff --check

The broader Slack run dynamically resolves every tests/**/*slack*.py file and passes all 33 files.

Verification

  • focused parent-edit/proof regressions: 26 passed
  • tests/gateway/test_slack.py: 260 passed
  • all dynamically resolved Slack test files: 33 files, 476 passed
  • Ruff check: passed
  • py_compile: passed
  • git diff --check: passed
  • fresh read-only exact-diff review: no blocking findings
  • current upstream main conflict probe: clean; no relevant overlap since the reviewed target snapshot

A local canonical full-run attempt did not pass: it reported 73 non-Slack test failures and 10 collection/import failures. No Slack test failed in that run, and the full-suite checklist remains unchecked. Upstream Actions remain the authority for the repository-wide and cross-platform result.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes
  • I've tested on my platform: macOS 26.3.1

Documentation & Housekeeping

  • I've updated relevant documentation — N/A; internal Slack event filtering only
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact — platform-independent Python event handling
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Screenshots / Logs

The regression tests model sanitized Slack envelopes directly; no workspace-specific logs or identifiers are included.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for isolating a real Slack ingress replay path. Current main still normalizes every message_changed event at plugins/platforms/slack/adapter.py:5251-5283, so filtering metadata-only parent updates is worthwhile.

Problems

  • The new classifier can drop a real visible removal. In the PR at plugins/platforms/slack/adapter.py:5284, a prior non-empty attachments/files/blocks value and an omitted current value is marked unknown, not changed. If reply metadata also changes, the return at the new reply-metadata guard suppresses the event. Treat a formerly non-empty visible field becoming missing/empty as a visible change, and add coverage for that case.
  • The test hunk is stale: its anchor was removed by 6b81590c55 from tests/gateway/test_slack.py; GitHub currently marks the branch DIRTY.

Suggested changes

  • Preserve visible field removals, then relocate the focused regressions into current test structure.

Automated hermes-sweeper review.

Comment thread plugins/platforms/slack/adapter.py Outdated
@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins platform/slack Slack app adapter labels Jul 30, 2026
Co-authored-by: codex <noreply@openai.com>
@nahyeongjin1
nahyeongjin1 force-pushed the fix/slack-thread-parent-replay branch from d57215a to abe5fce Compare July 30, 2026 23:34
Slack emits `message_changed` for existing thread parents. When the root
timestamp was absent from the process-local `_processed_message_ts` set,
the adapter normalized that edit as fresh inbound input and replayed
instructions the agent had already acted on.

Classify parent edits before normalization: filter metadata-only updates
without suppressing proven visible text, block, attachment, or file
changes, and suppress later edits of replied parents whose pre-edit
snapshot already addressed this workspace's bot by mention, Block Kit,
or configured wake pattern. A newly added mention still routes exactly
once when the previous snapshot reliably proves the bot was not
addressed; a missing, partial, or malformed `previous_message` fails
closed. Reply evidence and traversed Block Kit shapes are validated
before being used as proof, and bot identity stays scoped to the event
team.

Co-authored-by: codex <noreply@openai.com>
@0xble

0xble commented Aug 10, 2026

Copy link
Copy Markdown

I opened #83501 as a deliberately narrower, current-main alternative for the metadata-only replay path. It filters hidden thread-parent updates that change only Slack reply bookkeeping while preserving visible text, block, attachment, file, and newly-added mention changes. The broader answered-parent and bot-address-history policy in this PR remains a distinct follow-up rather than being silently replaced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have platform/slack Slack app adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants