Skip to content

fix(discord): reconcile ambiguous auto-thread creation - #73097

Open
izumi0uu wants to merge 2 commits into
NousResearch:mainfrom
izumi0uu:codex/fix-discord-thread-create-idempotency-73032
Open

fix(discord): reconcile ambiguous auto-thread creation#73097
izumi0uu wants to merge 2 commits into
NousResearch:mainfrom
izumi0uu:codex/fix-discord-thread-create-idempotency-73032

Conversation

@izumi0uu

Copy link
Copy Markdown
Contributor

What does this PR do?

Discord auto-thread creation is a remote side effect: message.create_thread() can commit on Discord and still raise locally if the response is lost. The existing exception path treats that as a definitive failure and immediately creates a second thread from a fallback seed message.

This PR reconciles the original starter-message thread before allowing the fallback. It reuses a matching thread from message.thread, the client cache, or Discord's channel fetch; retries the lookup briefly for delayed visibility; and skips the fallback when reconciliation itself is inconclusive. A fallback seed is still created when Discord confirms that the original thread is absent.

Related Issue

Fixes #73032

Related implementation: #73092 checks the in-memory message.thread path. This PR also covers cache misses, REST lookup, delayed visibility, mismatched cached channels, and lookup failures.

This preserves the original author commit from the closed #48685 and adapts it to current main.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • Tests (adding or improving test coverage)

Changes Made

  • Validate that a reconciled object is a Discord thread whose ID matches the starter message ID.
  • Resolve an already-created thread through the message object, client cache, and Discord REST fetch.
  • Retry reconciliation briefly before creating a fallback seed.
  • Only use the fallback after absence is confirmed; an inconclusive lookup returns failure rather than risking a duplicate remote side effect.
  • Cover message, cache, fetch, delayed-visibility, unrelated-channel, confirmed-absence fallback, and inconclusive-lookup paths.

How to Test

  1. Simulate a direct create that attaches the original-message thread and then raises ConnectionError.
  2. On upstream/main@fae29c841, observe one direct thread plus one fallback thread.
  3. On this branch, observe the original thread being reused with no fallback seed.
  4. Run:
./.venv/bin/python -m pytest \
  tests/gateway/test_discord_channel_controls.py \
  tests/gateway/test_discord_slash_commands.py \
  tests/gateway/test_discord_double_dispatch.py \
  tests/gateway/test_discord_free_response.py \
  tests/gateway/test_discord_thread_persistence.py \
  -q -o addopts=

./.venv/bin/ruff check \
  plugins/platforms/discord/adapter.py \
  tests/gateway/test_discord_channel_controls.py \
  tests/gateway/test_discord_slash_commands.py

Validation

Baseline harness: direct_creates=1, seed_messages=1, fallback_creates=1
Fixed harness:    direct_creates=1, seed_messages=0, fallback_creates=0
Related tests:    139 passed
Ruff:             passed
git diff --check: passed
Environment:      macOS 26.5.2, Python 3.11.15

Remaining Risk

The issue's duplicate-reply observation is real, but its separate ingress path was not isolated. Current main already has live-event deduplication, thread-starter pre-seeding, recovery deduplication, and persistent processing claims, so this PR intentionally does not change dispatch behavior without evidence. It fixes the deterministically reproduced duplicate-thread trigger.

Checklist

  • I've read the Contributing Guide.
  • My commit messages follow Conventional Commits.
  • I searched existing PRs and documented the relevant overlap above.
  • This PR contains only issue-related changes.
  • I've added and run focused regression tests.
  • I've tested on macOS 26.5.2 with Python 3.11.15.
  • Full pytest tests/ -q suite run (focused Discord coverage is reported above).
  • Documentation/config/schema changes are N/A.

@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/discord Discord bot adapter sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages needs-decision Awaiting maintainer decision before any implementation labels Jul 28, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #73092 handles the in-memory message.thread recovery path for #73032; this PR additionally covers cache misses, REST lookup, delayed visibility, and fail-safe inconclusive lookup. They overlap but are not redundant.

@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 the ambiguous-success create path. The production premise remains valid on current main: plugins/platforms/discord/adapter.py:6580-6590 catches a direct-create exception and immediately posts the fallback seed / creates its separate thread, with no reconciliation step.

Problems

  • The test portion needs manual salvage. This PR is based on ef267011, while current main pruned the insertion region in tests/gateway/test_discord_channel_controls.py (the file now reaches its config section at line 219). GitHub reports the PR CONFLICTING/DIRTY; the added tests cannot apply as-is.

Suggested changes

  • Preserve the adapter reconciliation logic and relocate its regression coverage into the remaining Discord auto-thread test surface, adapting the current fixture rather than restoring removed tests.

Automated hermes-sweeper review.

Comment thread tests/gateway/test_discord_channel_controls.py Outdated
@teknium1 teknium1 added 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
@izumi0uu
izumi0uu force-pushed the codex/fix-discord-thread-create-idempotency-73032 branch from 3f9afa9 to 39486ff Compare August 1, 2026 12:16
@izumi0uu

izumi0uu commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Addressed in d55bab7 and 39486ff.
Rebased onto current main, preserved the adapter reconciliation logic, and relocated the regression coverage into the surviving Discord auto-thread test surface in test_discord_slash_commands.py rather than restoring the pruned test_discord_channel_controls.py block.

@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Summary

Nine PRs address or reference this Discord auto-thread complex. Their diffs cover the original seed-message fallback, retry/fail-closed routing, orphan-seed cleanup, and, for #73032, reconciliation of an ambiguous direct-create result before any second thread is created.

Related pull requests

Duplicates

#20260 was superseded by merged #56108; #48690 duplicates #48685's core recovery mechanism; #52430 duplicates the tested canonical #52423; #73092 is a narrower subset of #73097. The target-fix lineage is #48685 -> #73097.

Suggested consolidation

Keep #73097 open with a salvage path centered on its adapter reconciliation logic and relocated ambiguous-success regression tests; this preserves the Verify-designated best fix without making a merge recommendation. Close #73092 as a duplicate of #73097 despite its keep_open maintainer-bot verdict because its diff covers only message.thread, while #73097 also covers cache/REST lookup, delayed visibility, confirmed absence, and inconclusive lookup; independently keep #52423 open for its #52422 cleanup, with #52430 remaining closed as its duplicate.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I73032(["issue #73032 (open)"])
    P73097["PR #73097 (open)"]
    P73097 -->|best fix| I73032
    class I73032 open
    class P73097 open
    class P73097 best
    class P73097 target
    click I73032 "https://github.com/NousResearch/hermes-agent/issues/73032"
    click P73097 "https://github.com/NousResearch/hermes-agent/pull/73097"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 9 pull requests and 3 issues in this complex. Each diff was read against this issue; Assessment working set: 60 kB of PR diffs, 33 kB of issue/PR text, 11 kB of discussion (14 comments), 23 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

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 needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have platform/discord Discord bot 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.

[Bug]: Discord auto-thread fallback can create duplicate threads and duplicate agent replies

5 participants