Repository navigation
fix(discord): retire opening-title aliases on moderator renames - #132758
andrexibiza wants to merge 9 commits into
Conversation
alt-glitch
added
type/bug
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
PR Review — #132758 (head 6f27ef0b)
Verdict: Request changes — the raw-event state machine is the right mechanism and the message-ID gap is a real find, but the PR's shape (octopus merge commit carrying the change, three open PRs stacked underneath) and its CI-blind headline tests need fixing before this can be considered for merge. Merge decision stays with the maintainer.
Premise, verified on origin/main (1298c8e): main has no _semantic_thread_renames at all, so #131614 itself only exists on #131243's head — Teknium's triage on the issue already rated it a P2 inside an unmerged fix; the P0 label here is inherited from #131242. What this PR adds over the composed base (#128788 + #131120 + #131243, octopus-merged onto main in a scratch worktree) is a net delta of ~76 LOC in adapter.py, ~20 LOC in gateway/session.py + run_agent_cache.py, and ~360 test lines.
Tests (hermetic, fake HOME): PR head 119 passed / 0 failed / 20 skipped across the 9 Discord/voice files. The PR's own test files copied onto the composed base: test_discord_turn_prompt_parity.py 16/16 red, test_discord_composed_pin_contract.py 4/4 red — so the delta does move behaviour. But test_discord_auto_thread_rename_pin.py was 16 skipped on BOTH trees — see Critical 1.
Critical
-
The 16 native-dispatch rename regressions never run in CI.
discord_sdk(tests/gateway/test_discord_auto_thread_rename_pin.py:84-91) doespytest.importorskip("discord"), anddiscord.pyis deliberately not in the[all]extra (pyproject.toml:328-332); the unittestsjob syncsall,anthropic,bedrock,…withoutmessaging(.github/workflows/tests.yml:63). Only the e2e job installsmessaging(tests.yml:145) and it runstests/e2e/**only. So the cases the PR body leads with ("prior fix fails 12 of the 22 native rename cases") are green locally in your PM env with the extra and silentlyseverywhere else — a later regression of_on_platform_raw_thread_updatewould pass CI. Fix direction: the state machine does not need the SDK — drive_on_platform_raw_thread_update(SimpleNamespace(thread_id=800, data={"name": …}))directly for the ordering/ownership invariants intests/gateway/, and keep ONE real-SDK case (twoTHREAD_UPDATEframes parsed before callbacks run) intests/e2e/core/platforms/next totest_discord_contract.py/_drv_discord.py, which is the lane that has the extra. -
Your change lives inside an octopus merge commit (
6f27ef0b, parents1298c8e+01435400+f86610fa). A rebase-merge drops merge commits, so landing this PR as the repo does (rebase, never squash) would either be refused or lose your delta; a--mergewould land three still-open P0 PRs (#128788, #131120, #131243) under this PR's title. Please rebuild the branch linearly: the stacked contributor commits as-is (authorship preserved), then your change as a normalfix(discord): …commit on top — or, since #131243 allows maintainer edits, offer the ~100 LOC production delta to its author as a follow-up commit there and keep this PR to the message-ID gap. Either way the PR body should say which of the three originals it supersedes; right now it states "no existing PR is closed or rewritten" while re-landing all of their commits.
Warnings
-
Test volume. 22 parametrized native cases + 4 composed + 16 parity variants for a ~100 LOC change. The invariants are three: (a) a moderator rename retires the record even when the second
THREAD_UPDATEis parsed before the first callback runs; (b) a return to Hermes's title after a moderator edit renders as itself; (c) typed / slash / voice turns pin the identical string with Discord tools enabled and disabled. One case per invariant (plus the failed/cancelled attempt ownership) is the bar here; the product grids mostly re-prove the same branch. -
Residual, worth a one-line comment in
_format_thread_chat_name: after a lost-event gap (re-IDENTIFY without RESUME repopulates thread names fromGUILD_CREATE, noTHREAD_UPDATE), a moderator who restored the opening name during the gap still reads as cache lag (thread_name == renamed.replaced,set_seenFalse), so their later pick of Hermes's title shows the opening name — the original #131614 shape, narrowed to that gap. Not a blocker; just don't let the docstring claim the raw path makes the record exact.
Looks good
_on_platform_raw_thread_updateis the correct seam: discord.py 2.7.1parse_thread_updatedispatchesraw_thread_updatebefore mutating the cachedThreadand in both the cached and uncached branches (verified in the installed package), so the payload name is an immutable snapshot and the before/after handler's two failure modes (mutatedafter,thread_joinfor uncached threads) go away.- Publishing the
_SemanticThreadRenamerecord beforeawait edit(...)and cleaning up by identity infinally(covers cancellation, protects a newer attempt) is right; the(event_name, new_name) == (replaced, set_name)transition check correctly admits only Hermes's own opening→title edge. - The message-ID change is a real gap in #128788, reproduced: with
_discord_tools_loaded()true, a typed turn (hasmessage_id) renders the "Triggering message" line and a slash/voice turn (message_id=None) does not, so the pinned prompt differs between alternating turns — exactly the #128787 flip, surviving #128788 for every user with thediscordtoolset enabled. Making the line static and dropping the presence bit from therun_agent_cache.pykey (gateway/run_agent_cache.py:730-737) is the correct fix and keeps the key covering everything rendered. I'll flag it on #128788 too. - Nix and Docker lanes already green on this head; CI unit lane approved and running.
…rompt inputs Native slash commands (/skill, /plan, /queue), the /thread starter and voice-channel input are human turns, so each re-pins the session-context prompt and the channel prompt. They built their event without the fields on_message sets: - a DM slash turn had no chat name, so the "origin" line rendered the channel id instead of the user's name; - a thread slash turn and the /thread starter dropped the parent channel from the thread label, and the starter took the parent channel's topic; - a spoken turn named the speaker by numeric id, not display name; - none carried the channel's bound skill, so a session opened by one of them never loaded it. Each switch between one of these turns and a typed message re-rendered the pinned context prompt and back again, a prompt-cache miss each way. They now use the same formatters (_format_thread_chat_name, _get_effective_topic on the thread, the member display name) and _resolve_channel_skills as the message path.
…e channel's current labels A spoken turn rebuilds its source from the copy `/voice join` stored, and three of its prompt inputs still differed from a typed message in the same conversation: - voice joined from a thread looked up the channel prompt and bound skills for the thread alone, while a typed message there falls back to the parent channel's entry, so a parent-only binding never reached speech; - the chat name and topic stayed at their join-time values, so after a rename or topic edit every typed/spoken switch re-rendered the pinned context prompt and back; - `guild.get_member()` is cache-only, and on a miss the speaker was named by numeric id even when they were the member who ran `/voice join`. The resolvers now get the stored parent id, the name and topic come from the bound channel through the same formatters a message uses (the stored values stay as the fallback when the channel is not cached), and an uncached joiner keeps the name bound at join; any other uncached speaker still gets their id, never the joiner's name. Reported by andrexibiza (F1, F2) and teknium1 (get_member) on NousResearch#128788. Co-authored-by: Matt McFarland <mattttmcfarland@gmail.com>
The first turn of an auto-threaded @mention already points its source at the new thread, but it read chat_topic from the text channel the mention was posted in. The thread's next message reads the thread's own topic (none, outside forums), so the pinned session-context prompt was re-rendered on turn 2 of every auto-threaded conversation in a channel with a topic: a prompt-cache miss, and a Channel Topic line the model saw once and then lost.
…efault mention gate The regression test disabled require_mention and sent an untagged parent message, so it never covered the default entrypoint the issue describes. The first message is now a real bot mention under the default gate, an untagged parent message is kept as a rejection control (no thread, no dispatch), and the module docstring no longer implies the channel prompt / skill lookups were part of the bug.
…prompt chat_name keys the pinned session-context prompt, and the title lane renames a new auto-thread once the LLM title arrives, normally before the user's second message. Turn 2 read the new name and re-rendered the already-sent prompt, a prompt-cache miss on every auto-threaded conversation on the default config. rename_thread now records the name it replaced and the name it set for a guarded semantic rename, and _format_thread_chat_name keeps the replaced name while the thread still carries Hermes's title. A rename by anyone else no longer matches and re-renders as before.
…names the thread The mask from Hermes's own title rename matched on name text only and was never retired, so a moderator who renamed the thread away and later restored Hermes's title saw the prompt show the thread's opening name instead. Once a name other than Hermes's title has been observed the record is dropped; before Hermes's title is first seen, the replaced name is still tolerated because a cache that has not caught up with the edit reports it. Fixes NousResearch#131614
Track rename-attempt ownership before the HTTP edit and retire semantic opening-title aliases when an immutable raw update observes an external change. Failed or cancelled older attempts cannot remove newer records. Keep triggering message IDs and their presence out of pinned system bytes. Run ownership regressions without the optional Discord SDK; exercise one real Bot/parser ordering contract in the messaging E2E lane. Remove the redundant test cross-products and explain the missed-event reconnect limit. Original discovery and published repair: Axl Ibiza, MBA NousResearch#131243 (comment) NousResearch#131243 (review) Built on John Paul Soliva's seven contributor commits, replayed linearly with original authorship and Matt McFarland's co-author trailer preserved.
…e_id presence With the discord toolset loaded, a typed turn rendered the per-turn triggering-message line and keyed the agent cache on message_id presence, while slash, /thread and voice turns (no triggering message) did not, so the pinned session-context prompt re-rendered on every switch between them. The line is now static and the presence bit is out of the key; the real id still rides the user message when there is one. Reported by teknium1 in the NousResearch#132758 review; fix shape from andrexibiza's NousResearch#132758. Co-authored-by: Andrex Ibiza, MBA <andrexibiza@gmail.com>
2bc7358 to
a8368ce
Compare
|
@teknium1 The changes requested in your review are now on the current #132758 head
Both inline threads are resolved. On this rebased head, the related local suites returned 224 passed/4 skipped without the SDK and 225 passed/4 skipped with discord.py 2.7.1; focused Ruff and diff checks passed. Hosted CI, Nix and Docker are still |
|
Verified at a8368ce: discord + voice suites pass 82/82 with 4 skipped. |
The mask record was installed only after the REST edit returned and was retired from the cached on_thread_update callback. Gateway updates can land while the edit is in flight, and discord.py mutates the cached thread before scheduling callbacks, so batched updates all report the final name. Either way a moderator's restore of Hermes's title could render the opening name. Record the guarded attempt before awaiting the edit, advance or retire it from on_raw_thread_update, and on failure or cancellation drop only the record that attempt still owns. The record store and _format_thread_chat_name move into adapter_thread_titles.py so adapter.py does not grow. Rename-only part of #132758's f67ef41; its message-id, slash/voice and composed-contract changes are left to their own PRs. Co-authored-by: Andrex Ibiza, MBA <andrexibiza@gmail.com>
|
Thanks @andrexibiza for this work and for the review findings that shaped the final fix. The rename part of I'm closing this as superseded for the rename fix. The other changes here are not in #133393. The message-ID presence fix and the slash/voice label belong with #128788, as Teknium suggested, and the topic re-key belongs with #131120. Please carry them over there if they aren't already. |
…e_id presence With the discord toolset loaded, a typed turn rendered the per-turn triggering-message line and keyed the agent cache on message_id presence, while slash, /thread and voice turns (no triggering message) did not, so the pinned session-context prompt re-rendered on every switch between them. The line is now static and the presence bit is out of the key; the real id still rides the user message when there is one. Reported by teknium1 in the #132758 review; fix shape from andrexibiza's #132758. Co-authored-by: Andrex Ibiza, MBA <andrexibiza@gmail.com> (cherry picked from commit 2a3f04a)
What does this PR do?
Keep a Discord auto-thread's opening label pinned while Hermes owns its uninterrupted rename attempt. An observed moderator rename retires that alias, including a rename away and back, batched gateway updates, and edits received before the HTTP request completes. Typed, slash and voice turns share the same prompt prefix even when Discord message tools are enabled.
Original discovery and published rename-state repair: Axl Ibiza. Sources: Axl's original report, published repair and regressions, and raw-event/in-flight ordering explanation. #131614 records that reporting credit.
This integrates John Paul Soliva's auto-thread/title/topic and slash/voice metadata work. His existing authored commits and Matt McFarland's co-authorship remain in the published ancestry.
Related Issue and PR scope
Fixes #131614. Related: #131242.
This PR supersedes the implementations in #128788, #131120 and #131243: it includes their contributor changes, then adds raw rename-attempt ownership and the tools-enabled triggering-message instruction repair. Those original PRs have not been closed by this update. #132288 and #132295 are overlapping label-masking drafts.
The linear-history request is now published. This PR head is a nine-commit, single-parent series rebased onto main at
af90026aa09949579bd423d24def3d38f743cde0: seven contributor commits retain their authors, author dates, full messages and co-author trailer, followed by two repair/test commits. The old published head is preserved atandrexibiza/hermes-agent:backup/pr132758-before-linear-2bc73582; the rebased candidate is also preserved atbackup/pr132758-linear-current-af90026. The code delta composes cleanly with that main pin. Current-head upstream CI still requires approval and has not executed.Type of Change
Changes Made
THREAD_UPDATEsnapshots advance Hermes's opening-to-generated transition or retire an observed external change. A late HTTP success cannot resurrect a retired record.connect()registration with two moderator frames parsed before callbacks run and no platform-event subscriber.The formatter now explains the accepted reconnect limitation: re-IDENTIFY without RESUME can repopulate a title without replaying its raw rename event. An unseen restoration to the opening name can remain indistinguishable from cache lag; the raw path does not establish provenance for missed events.
How to Test
Use the canonical runner in an isolated PM test environment, with retries disabled. The gateway checks run without the Discord SDK:
Run the native witness in the messaging-enabled environment:
af90026— three gateway files, without Discord installed, Python 3.11.11The earlier broad receipts used Python 3.14.7 and the pre-rebase main. The current rebased head was rerun independently on Windows/Python 3.11.11 with retries disabled: 15 files without the SDK and 16 with an isolated discord.py 2.7.1 install. Four cases remain skipped on this host even with the SDK; the warnings include unawaited AsyncMock coroutines in existing voice tests. Focused Ruff and
git diff --checkpass on the current tree. Final tree:cc097281315f186f23744cdb970d000e410c3ac9, published head:a8368ceef373966b8caa6024166ca7ea8b99f77c.The revised gateway tests produce 10 failures on the previous composed implementation without installing Discord. The native SDK witness also fails on that implementation at the restored-generated-title assertion and passes with this repair.
Remote CI is not green yet. On the rebased current head, CI, Nix, and Docker report
action_required; upstream fork-workflow approval is required. Local results do not replace these checks.Checklist
Coverage and runtime relationship
This Mermaid source was rendered with Mermaid CLI using the dark theme and the PNG was visually inspected.
%%{init: {"theme":"dark"}}%% flowchart TB subgraph FLOW["Coverage and runtime relationship"] direction TB U["SDK-free gateway invariants"]:::context E["Messaging E2E: real Bot and batched frames"]:::context R["Raw THREAD_UPDATE snapshots"]:::entry O["Adapter owns the rename attempt"]:::owner F["Formatter uses the active alias or actual title"]:::transition P["Typed, slash and voice prompt parity"]:::context G["Missed events after re-IDENTIFY remain ambiguous"]:::context U --> R E --> R R --> O --> F --> P G -.-> F end LEGEND["Legend<br/>Blue: entry / incoming<br/>Purple: ownership / policy<br/>Amber: transition / pending<br/>Gray: context / transient<br/>Coverage evidence: see test results"] FLOW ~~~ LEGEND classDef entry fill:#172554,stroke:#60a5fa,color:#eff6ff,stroke-width:2px classDef transition fill:#451a03,stroke:#fbbf24,color:#fffbeb,stroke-width:2px classDef owner fill:#3b0764,stroke:#c084fc,color:#faf5ff,stroke-width:2px classDef context fill:#1e293b,stroke:#94a3b8,color:#f8fafc,stroke-width:2px style FLOW fill:#0f172a,stroke:#475569,color:#f8fafc style LEGEND fill:#1e293b,stroke:#94a3b8,color:#f8fafc,font-size:11px click U "https://github.com/NousResearch/hermes-agent/blob/a8368ceef373966b8caa6024166ca7ea8b99f77c/tests/gateway/test_discord_auto_thread_rename_pin.py#L100" "Pinned source from the existing node-source table" click E "https://github.com/NousResearch/hermes-agent/blob/a8368ceef373966b8caa6024166ca7ea8b99f77c/tests/e2e/core/platforms/test_discord_thread_rename_ordering.py#L20" "Pinned source from the existing node-source table" click R "https://github.com/NousResearch/hermes-agent/blob/a8368ceef373966b8caa6024166ca7ea8b99f77c/plugins/platforms/discord/adapter.py#L1728" "Pinned source from the existing node-source table" click O "https://github.com/NousResearch/hermes-agent/blob/a8368ceef373966b8caa6024166ca7ea8b99f77c/plugins/platforms/discord/adapter.py#L5451" "Pinned source from the existing node-source table" click F "https://github.com/NousResearch/hermes-agent/blob/a8368ceef373966b8caa6024166ca7ea8b99f77c/plugins/platforms/discord/adapter.py#L5848" "Pinned source from the existing node-source table" click G "https://github.com/NousResearch/hermes-agent/blob/a8368ceef373966b8caa6024166ca7ea8b99f77c/plugins/platforms/discord/adapter.py#L5848" "Pinned source from the existing node-source table" click P "https://github.com/NousResearch/hermes-agent/blob/a8368ceef373966b8caa6024166ca7ea8b99f77c/tests/gateway/test_discord_turn_prompt_parity.py#L99" "Pinned source from the existing node-source table"