-
Notifications
You must be signed in to change notification settings - Fork 52.7k
feat(discord): add history_full_thread option to bypass self-message partition #51414
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6188,10 +6188,14 @@ def _discord_history_backfill(self) -> bool: | |
| def _discord_history_backfill_limit(self) -> int: | ||
| """Return the max number of messages to scan backwards for context. | ||
|
|
||
| In practice the scan usually stops much earlier — at the bot's own | ||
| last message in the channel (the natural partition point). This | ||
| limit is a safety cap for cold starts and long gaps where no prior | ||
| bot message exists in recent history. | ||
| In the default (partition) mode, the scan usually stops much earlier — | ||
| at the bot's own last message in the channel (the natural partition | ||
| point). This limit is a safety cap for cold starts and long gaps | ||
| where no prior bot message exists in recent history. | ||
|
|
||
| When ``full_thread`` mode is enabled, the scan walks the entire | ||
| thread up to this limit without stopping at the partition point, so | ||
| the agent sees the full conversation surrounding the trigger. | ||
| """ | ||
| configured = self.config.extra.get("history_backfill_limit") | ||
| if configured is not None: | ||
|
|
@@ -6205,6 +6209,31 @@ def _discord_history_backfill_limit(self) -> int: | |
| except (ValueError, TypeError): | ||
| return 50 | ||
|
|
||
| def _discord_history_full_thread(self) -> bool: | ||
| """Return whether the bot should fetch the full thread instead of | ||
| stopping at the most recent self-message partition point. | ||
|
|
||
| Default: ``False`` (preserves existing behaviour — only the messages | ||
| since the bot's last reply are surfaced, matching the conversation | ||
| transcript window). | ||
|
|
||
| When enabled, the scan walks the entire thread (up to | ||
| ``history_backfill_limit``) without stopping at the partition point. | ||
| Useful when the user wants the bot to see the full thread context — | ||
| e.g. when resuming a long-running investigation in a thread where | ||
| the bot has replied multiple times. | ||
|
|
||
| Trade-off: the bot sees more context, but messages that are already | ||
| 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") | ||
| if configured is not None: | ||
| if isinstance(configured, str): | ||
| return configured.lower() in {"true", "1", "yes", "on"} | ||
| return bool(configured) | ||
| return os.getenv("DISCORD_HISTORY_FULL_THREAD", "false").lower() in {"true", "1", "yes", "on"} | ||
|
|
||
| async def _fetch_channel_context( | ||
| self, | ||
| channel: Any, | ||
|
|
@@ -6217,6 +6246,13 @@ async def _fetch_channel_context( | |
| a message sent by this bot (the natural partition point between | ||
| bot turns) or reaches ``history_backfill_limit``. | ||
|
|
||
| When ``history_full_thread`` is enabled, the partition-stop on the | ||
| bot's own messages is skipped: the scan walks the entire thread up | ||
| to ``history_backfill_limit`` so the agent sees the full thread | ||
| context. This is useful for resuming a long thread where the bot | ||
| has already replied multiple times — without this mode, only the | ||
| messages since the most recent bot reply are surfaced. | ||
|
|
||
| When ``reply_target`` is provided (the user replied to a specific | ||
| message), a second backward scan is run ending at that target so the | ||
| agent sees the conversation surrounding what the user pointed at — | ||
|
|
@@ -6235,6 +6271,7 @@ async def _fetch_channel_context( | |
| limit = self._discord_history_backfill_limit() | ||
| if limit <= 0: | ||
| return "" | ||
| full_thread = self._discord_history_full_thread() | ||
|
|
||
| # Determine which bot messages to include in context | ||
| allow_bots_raw = os.getenv("DISCORD_ALLOW_BOTS", "none").lower().strip() | ||
|
|
@@ -6247,14 +6284,22 @@ async def _fetch_channel_context( | |
| # Guard: only use the cache when it's chronologically before the | ||
| # trigger — Discord snowflake IDs are monotonically increasing, so | ||
| # a simple int comparison suffices. | ||
| # | ||
| # full_thread mode bypasses the cache entirely: the user wants the | ||
| # entire thread context, including messages that preceded prior bot | ||
| # replies. Passing `after=_last_self_message_id` here would silently | ||
| # drop everything before our last response, defeating the feature on | ||
| # the hot path. See regression test | ||
| # `test_fetch_channel_context_full_thread_ignores_last_self_cache`. | ||
| channel_id = str(getattr(channel, "id", "")) | ||
| _cached_id = self._last_self_message_id.get(channel_id) | ||
| _after_obj = None | ||
| try: | ||
| if _cached_id and int(_cached_id) < int(before.id): | ||
| _after_obj = discord.Object(id=int(_cached_id)) | ||
| except (ValueError, TypeError): | ||
| pass # Malformed cache entry — fall back to cold-start scan | ||
| if not full_thread: | ||
| _cached_id = self._last_self_message_id.get(channel_id) | ||
| try: | ||
| if _cached_id and int(_cached_id) < int(before.id): | ||
| _after_obj = discord.Object(id=int(_cached_id)) | ||
| except (ValueError, TypeError): | ||
| pass # Malformed cache entry — fall back to cold-start scan | ||
|
|
||
| is_thread_channel = isinstance(channel, discord.Thread) | ||
| has_unverified = False | ||
|
|
@@ -6347,7 +6392,10 @@ def _keep(msg) -> Optional[str]: | |
| # partition point. Everything before this is already in the | ||
| # session transcript. (Redundant when _after_obj is set, but | ||
| # needed for cold start.) | ||
| if msg.author == self._client.user: | ||
| # 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: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Valid catch — thanks. The local Fixed in this push:
Env var fallback ( Ready for re-review on this thread.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Re-ping after rebase: this concern is now resolved at the new location of the cache-construction guard. See commit |
||
| break | ||
| line = _keep(msg) | ||
| if line is None: | ||
|
|
@@ -9718,6 +9766,12 @@ def _apply_yaml_config(yaml_cfg: dict, discord_cfg: dict) -> dict | None: | |
| hbl = discord_cfg.get("history_backfill_limit") | ||
| if hbl is not None and not os.getenv("DISCORD_HISTORY_BACKFILL_LIMIT"): | ||
| os.environ["DISCORD_HISTORY_BACKFILL_LIMIT"] = str(hbl) | ||
| # history_full_thread: opt-in override that walks the entire thread | ||
| # instead of stopping at the bot's most recent self-message partition. | ||
| # Default in the adapter is False, so existing deployments see no | ||
| # behaviour change unless they explicitly set this in config.yaml. | ||
| if "history_full_thread" in discord_cfg and not os.getenv("DISCORD_HISTORY_FULL_THREAD"): | ||
| os.environ["DISCORD_HISTORY_FULL_THREAD"] = str(discord_cfg["history_full_thread"]).lower() | ||
| # allow_mentions: granular control over what the bot can ping. | ||
| # Safe defaults (no @everyone/roles) are applied in the adapter; | ||
| # these YAML keys only override when set and let users opt back | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR's documented
discord.history_full_threadkey is not seeded intoPlatformConfig.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right — the docstring on
_discord_history_full_threadadvertisedconfig.extra["history_full_thread"]but the YAML→env bridge in_apply_yaml_configonly knew abouthistory_backfillandhistory_backfill_limit. Setting the documented YAML key was a dead letter.Fixed in this push:
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.hermes_cli/config.pyDEFAULT_CONFIG["discord"]("history_full_thread": False) sohermes config show/hermes setupreflect it.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.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_threaddefaults toFalse, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-ping after rebase: this concern is now resolved — the YAML wiring the previous version added is preserved through the rebase.
history_full_threadis still seeded intoDEFAULT_CONFIGinhermes_cli/config.py, threaded through_apply_yaml_configinplugins/platforms/discord/adapter.py, and documented in bothwebsite/docs/user-guide/messaging/discord.mdand the ZH translation. Re-requesting your re-review on head723212debd.