Skip to content

fix(streaming): add Slack streaming=false default to match Discord - #37688

Closed
AhmetArif0 wants to merge 1 commit into
NousResearch:mainfrom
AhmetArif0:fix/slack-streaming-default
Closed

fix(streaming): add Slack streaming=false default to match Discord#37688
AhmetArif0 wants to merge 1 commit into
NousResearch:mainfrom
AhmetArif0:fix/slack-streaming-default

Conversation

@AhmetArif0

Copy link
Copy Markdown
Contributor

Summary

PR #37303 added per-platform streaming defaults and its commit message explicitly states: "Discord/Slack/etc. only have edit-based streaming (repeated editMessage), which flickers and is noticeably jankier" — but only discord.streaming=false was shipped. Slack uses the same edit-based streaming mechanism and has the same flicker problem, yet it was left to follow the global switch (defaults to true when streaming is enabled globally).

Changes

  • hermes_cli/config.py: add "slack": {"streaming": False} to DEFAULT_CONFIG["display"]["platforms"] alongside the existing Discord default. Updated the comment to mention both Discord and Slack. The same deep-merge semantics apply: a user who explicitly sets display.platforms.slack.streaming: true keeps their value unchanged.
  • tests/gateway/test_per_platform_streaming_defaults.py: update all existing assertions to also cover slack, rename the resolver test to reflect both platforms, and add slack.streaming to the dashboard schema exposure check.

Why this is a gap-filler

Like Discord, the Slack default is a gap-filler: deep-merge means any explicit user value wins. Adding the default only affects users who have streaming globally enabled but have not explicitly overridden the Slack platform setting — exactly the set of users who would see the flickery edit-based streaming they didn't opt into.

Test plan

  • pytest tests/gateway/test_per_platform_streaming_defaults.py -x -q — 4 tests pass

PR NousResearch#37303 added per-platform streaming defaults and the commit message
explicitly called out "Discord/Slack/etc. only have edit-based streaming
(repeated editMessage), which flickers and is noticeably jankier" — but
only discord.streaming=false was shipped. Slack uses the same edit-based
streaming mechanism and has the same flicker problem, yet it was left to
follow the global switch (default true when streaming is enabled).

Add "slack": {"streaming": False} to DEFAULT_CONFIG["display"]["platforms"]
alongside the Discord default. The same deep-merge semantics apply: a user
who explicitly sets display.platforms.slack.streaming: true keeps their
value unchanged. The dashboard schema gains a slack.streaming toggle
automatically since it is generated from DEFAULT_CONFIG.

Update test_per_platform_streaming_defaults.py to cover slack in all
existing assertions and rename the resolver test to reflect both platforms.
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard area/config Config system, migrations, profiles platform/slack Slack app adapter labels Jun 2, 2026

@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 identifying the Slack edit-streaming parity gap. The current patch does not reach the gateway's runtime resolver.

Problems

  • gateway/run.py:18050 resolves streaming from _load_gateway_config(), but _load_gateway_config() reads raw YAML rather than merging DEFAULT_CONFIG (gateway/run.py:2315-2350). Therefore the new DEFAULT_CONFIG["display"]["platforms"]["slack"] entry is absent at runtime for users who have not written that setting.
  • With raw config, Slack inherits _TIER_MEDIUM["streaming"] = None through _PLATFORM_DEFAULTS (gateway/display_config.py:81-89,133), so the gateway follows global streaming. The proposed test instead passes a DEFAULT_CONFIG copy into the resolver, which does not exercise that path.

Suggested changes

  • Put the built-in Slack streaming default in gateway/display_config.py's _PLATFORM_DEFAULTS, where it applies after explicit raw-user overrides.
  • Test empty/raw config resolving Slack to False, and an explicit Slack true override resolving to True.

This is an automated hermes-sweeper review.

Comment thread hermes_cli/config.py
"platforms": {
"telegram": {"streaming": True},
"discord": {"streaming": False},
"slack": {"streaming": False},

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.

This DEFAULT_CONFIG entry is not consumed by gateway streaming: gateway/run.py:18050 passes _load_gateway_config() to the resolver, and _load_gateway_config() reads raw YAML without merging DEFAULT_CONFIG (gateway/run.py:2315-2350). Please place the built-in Slack default in gateway/display_config.py's _PLATFORM_DEFAULTS and test the raw-config path.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users area/streaming Streaming responses: gateway delivery, provider wire labels Jul 13, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Closing as superseded by #70196 (merged): streaming default parity is a behavior-default change held for an explicit maintainer decision — may still land separately.

Thanks for the work — it's credited in #70196's summary.

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

Labels

area/config Config system, migrations, profiles area/streaming Streaming responses: gateway delivery, provider wire comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have platform/slack Slack app adapter sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants