feat(discord): add history_full_thread option to bypass self-message partition - #51414
feat(discord): add history_full_thread option to bypass self-message partition#51414taras-polishchuk wants to merge 2 commits into
Conversation
alt-glitch
added
type/feature
tonydwb
left a comment
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Add history_full_thread option to bypass self-message partition.
Looks Good
- Clean feature addition with clear documentation
- Config-driven (extra config or env var)
- Properly handles the trade-off (more context vs token cost)
- Default preserves existing behavior (False)
- Well-structured with clear precedence rules
Reviewed by Hermes Agent
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused Discord context option. The underlying limitation still exists on current main (plugins/platforms/discord/adapter.py:5133-5138), but two changes are needed before this implements the requested behavior.
Problems
- The full-thread branch only bypasses the local
break(adapter.py:4431on this PR), while the existing hot-path cache still passesafter=_last_self_message_idintochannel.history()(adapter.py:4350-4354,4410). Since normal sends populate that cache (adapter.py:2092on current main), older messages are excluded before the loop can see them. - The PR advertises
discord.history_full_thread, but changes neither the YAML bridge nor docs. The existing bridge covers onlyhistory_backfillandhistory_backfill_limit(adapter.py:8313-8317on current main), so the documented YAML key will not activate this code path.
Suggested changes
- Disable the
aftercache boundary in full-thread mode and add a cached-path regression test. - Add the config default, YAML bridge, and Discord documentation; keep configuration user-facing through
config.yaml.
Automated hermes-sweeper review.
| # Skip this stop when full_thread mode is enabled — the user | ||
| # wants the entire thread context, even messages preceding | ||
| # prior bot replies. | ||
| if not full_thread and msg.author == self._client.user: |
There was a problem hiding this comment.
full_thread still uses the earlier _last_self_message_id cache as channel.history(after=...) (lines 4350-4354 and 4410). On the usual hot path, messages before the last bot response are therefore excluded before this condition runs. Disable that cache boundary in full-thread mode and add a regression test that seeds _last_self_message_id.
There was a problem hiding this comment.
Valid catch — thanks. The local break-skip on its own was a no-op on the hot path: by the time control reached the loop, channel.history() had already been narrowed by after=self._last_self_message_id[...] (adapter.py:4350-4354 and 4410), so the messages I wanted to walk past were filtered out before the partition check ever ran. Cold start works as advertised, hot path didn't.
Fixed in this push:
- Moved the bypass to the cache-construction site: in
_fetch_channel_context, the_after_objassignment is now wrapped inif not full_thread:so the cache is only consulted in partition mode. Same shape as the existingif not full_thread and msg.author == self._client.user: breakguard, just applied at the right layer. - Added
test_fetch_channel_context_full_thread_ignores_last_self_cacheintests/gateway/test_discord_free_response.py— seeds_last_self_message_id["888"] = "150"and assertschannel.history()is called withafter=Nonewhilehistory_full_thread: True. Without the fix the recordedafteris adiscord.Object(id=150). - Added a counterpart
test_fetch_channel_context_partition_mode_still_uses_cacheso a future refactor that disables the cache globally (in either direction) gets caught — partition mode keeps the optimisation intact.
Env var fallback (DISCORD_HISTORY_FULL_THREAD) is kept as the ops-escape hatch, with explicit env-var precedence over YAML in the bridge — same convention as DISCORD_HISTORY_BACKFILL and friends.
Ready for re-review on this thread.
There was a problem hiding this comment.
Re-ping after rebase: this concern is now resolved at the new location of the cache-construction guard. See commit 477e3f01a6 (rebased as feat(discord): add history_full_thread option) which still wraps the _after_obj assignment in if not full_thread:. The regression test test_fetch_channel_context_full_thread_ignores_last_self_cache (still passing on the rebased branch) seeds _last_self_message_id and asserts the cache is bypassed in full-thread mode. Re-requesting your re-review on the rebased head 723212debd.
| in the session transcript are duplicated in the prompt. Token cost | ||
| scales linearly with the active window. | ||
| """ | ||
| configured = self.config.extra.get("history_full_thread") |
There was a problem hiding this comment.
The PR's documented discord.history_full_thread key is not seeded into PlatformConfig.extra: this PR does not update _apply_yaml_config, DEFAULT_CONFIG, or the Discord docs. Wire the YAML key through the existing config bridge; behavioral settings must be user-facing in config.yaml rather than only in a new environment variable.
There was a problem hiding this comment.
You're right — the docstring on _discord_history_full_thread advertised config.extra["history_full_thread"] but the YAML→env bridge in _apply_yaml_config only knew about history_backfill and history_backfill_limit. Setting the documented YAML key was a dead letter.
Fixed in this push:
- Wired
discord.history_full_threadthrough_apply_yaml_config(adapter.py, alongside the existinghistory_backfill_limitblock) — samenot os.getenv(...)precedence guard as the neighbouring keys, so env var still wins over YAML when set. - Added the default to
hermes_cli/config.pyDEFAULT_CONFIG["discord"]("history_full_thread": False) sohermes config show/hermes setupreflect it. - Added regression coverage in
tests/gateway/test_config.py:test_bridges_discord_history_full_thread_from_config_yaml— happy path.test_history_full_thread_yaml_bridge_respects_existing_env_var— env-var precedence contract.test_history_full_thread_yaml_bridge_accepts_false_value— explicitfalsepropagates instead of being silently dropped.
- Documented at
website/docs/user-guide/messaging/discord.md(and thezh-Hanstranslation): new#### discord.history_full_threadsection with the type, default, YAML example, trade-offs (token cost + cache bypass), and the env-var override. Quick-block at the top of the file'sdiscord:snippet also lists the key. cli-config.yaml.examplegot the matching commented line in the platform block.
The PR's headline behaviour is unchanged — history_full_thread defaults to False, partition mode stays as-is, env var remains a fallback — but the YAML key is now actually load-bearing.
Re-review on both threads welcome.
There was a problem hiding this comment.
Re-ping after rebase: this concern is now resolved — the YAML wiring the previous version added is preserved through the rebase. history_full_thread is still seeded into DEFAULT_CONFIG in hermes_cli/config.py, threaded through _apply_yaml_config in plugins/platforms/discord/adapter.py, and documented in both website/docs/user-guide/messaging/discord.md and the ZH translation. Re-requesting your re-review on head 723212debd.
teknium1
added
the
sweeper:blast-moderate
…partition Adds an opt-in `history_full_thread` config / DISCORD_HISTORY_FULL_THREAD env var that disables the self-message partition-stop in `_fetch_channel_context`. When enabled, the bot walks the entire thread up to `history_backfill_limit` instead of stopping at its most recent reply, so the agent sees the full thread context on each trigger. Default is unchanged (`false`) — preserves existing behaviour and prompt-cache layout for all current users. Use case: long-running investigation threads where the bot has already replied multiple times. Without this flag, the user only sees messages since the most recent bot reply; with it, the user sees the full thread including earlier exchanges. Trade-off: messages already in the session transcript are duplicated in the prompt. Token cost scales linearly with the active window. The `history_backfill_limit` cap (default 50, raised to 200 in this PR's deployment) bounds the worst case. Tests: 4 new cases in test_discord_free_response.py covering the new mode, the default-still-partitioned regression, env-var override, and limit enforcement. 94/94 Discord adapter tests pass.
…ast-self cache Resolves review feedback on NousResearch#51414 from teknium1. Two changes, both required before the feature implements its advertised behaviour: 1. _last_self_message_id cache bypass on hot path. The previous patch only skipped the in-loop partition break, so on the usual send path channel.history() had already been narrowed by after=self._last_self_message_id[...] before the loop ran. The full thread was silently filtered before the partition check could apply. Move the guard to the cache-construction site: when history_full_thread is on, the _after_obj stays None and channel.history() gets the cold- start scan. Partition mode is unchanged. test_fetch_channel_context_full_thread_ignores_last_self_cache seeds _last_self_message_id with a real value and asserts channel.history() is called with after=None while history_full_thread is on. A counter- test guards the partition path so a future refactor can't silently disable the cache globally. 2. YAML -> env bridge for discord.history_full_thread. The PR's docstring advertised config.extra['history_full_thread'] but _apply_yaml_config only knew history_backfill and history_backfill_limit. Users who set the documented YAML key got a dead letter. Wire the key through with the same not os.getenv(...) precedence guard used by the neighbouring keys; env var remains a fallback for ops toggles. Defaults to False in hermes_cli/config.py DEFAULT_CONFIG so hermes config show reflects the new key. Regression coverage in tests/gateway/test_config.py covers the bridge, env-var precedence, and explicit false propagation. website/docs/user-guide/messaging/discord.md (and the zh-Hans translation) document the new key, the trade-offs (token cost, cache bypass), and the env-var escape hatch. cli-config.yaml.example lists the commented key in the platform block. Behaviour preserved: history_full_thread still defaults to False; partition mode is untouched. All 121 tests in test_discord_free_response.py and test_config.py pass.
03af2fe to
723212d
Compare
Rebase complete — ready for re-reviewRebased onto current Conflict resolution (3 files)main added a
Identity fixThe original Both SHAs are new (re-written commits); the previous Tests on rebased branch
— Taras |
4 participants
Summary
Adds an opt-in
history_full_threadconfig /DISCORD_HISTORY_FULL_THREADenv var that disables the self-message partition-stop in_fetch_channel_context. When enabled, the bot walks the entire thread up tohistory_backfill_limitinstead of stopping at its most recent reply, so the agent sees the full thread context on each trigger.Default is unchanged (
false) — preserves existing behaviour, prompt-cache layout, and token economics for all current users.Motivation
Long-running investigation threads where the bot has replied multiple times. Today, after the bot's most recent reply, only the messages since that reply are surfaced to the agent on each trigger — earlier exchanges in the same thread are invisible, even though they're in the same Discord thread.
Use cases:
Trade-off
Messages already in the session transcript are duplicated in the prompt. Token cost scales linearly with the active window.
history_backfill_limit(default 50) bounds the worst case — set to 200 in the operator's deployment.Configuration
Or via env:
DISCORD_HISTORY_FULL_THREAD=1Tests
tests/gateway/test_discord_free_response.py:test_fetch_channel_context_full_thread_walks_past_self_messages— happy path, full thread traversed in chronological ordertest_fetch_channel_context_full_thread_default_is_partition_mode— regression guard: default must remain partition mode (no silent-on-by-default change)test_fetch_channel_context_full_thread_env_var_overrides_default— runtime toggle via env without config edittest_fetch_channel_context_full_thread_respects_limit—history_backfill_limitcap honoured in the new modeRisk
Low. Feature is opt-in, defaults preserved, existing prompt-cache layout unchanged for the 99% of users who don't flip the flag. The new code path is one boolean short-circuit on an existing break-statement.
Diff: +44 -3 lines in adapter.py, +147 -1 lines in tests.