Skip to content

fix(discord): align clarify button lifetime - #97643

Open
apellis wants to merge 1 commit into
NousResearch:mainfrom
apellis:fix/discord-clarify-lifetime
Open

apellis wants to merge 1 commit into
NousResearch:mainfrom
apellis:fix/discord-clarify-lifetime

Conversation

@apellis

@apellis apellis commented Aug 29, 2026

Copy link
Copy Markdown

What does this PR do?

Discord native-choice clarify prompts currently have two independent lifetimes:

  • ClarifyChoiceView uses approvals.discord_prompt_timeout (300 seconds by default, capped at 900 seconds).
  • The gateway waiter uses agent.clarify_timeout (3600 seconds by default; <= 0 means unlimited).

After five minutes, Discord disables the buttons and says the prompt expired even though Hermes may still be waiting for up to another 55 minutes—or forever when clarify is configured as unlimited.

This PR makes the Discord clarify view use the same canonical clarify timeout as the gateway waiter:

  • positive timeout: the buttons and waiter share the same lifetime;
  • non-positive timeout: discord.ui.View(timeout=None), matching the documented unlimited wait;
  • execution approvals and other security-sensitive Discord views continue using the separately bounded approvals.discord_prompt_timeout.

It also fixes the cancellation sentinel for an unlimited wait. A session interruption previously produced [user did not respond within 0m]; it now reports [clarify wait cancelled before user response].

Related work

Related but intentionally different from #72742.

#72742 keeps the five-minute Discord view deadline, then introduces a second typed-answer grace timer and eventually resolves the waiter with an empty response. This PR instead removes the deadline split at its source. That preserves the documented agent.clarify_timeout <= 0 unlimited contract and avoids another timer/task/race lifecycle.

Type of change

  • Bug fix

Changes

  • Add _read_discord_clarify_timeout() using the canonical clarify resolver.
  • Use it only for ClarifyChoiceView; approval/confirmation views stay bounded.
  • Distinguish unlimited-wait cancellation from a finite timeout.
  • Add regressions for unlimited Discord view lifetime and the false 0m sentinel.

Test plan

The new tests were first run against the old behavior and failed as expected:

  • Discord view timeout was 300, not None.
  • unlimited-wait cancellation returned [user did not respond within 0m].

After the fix:

scripts/run_tests.sh tests/gateway/test_discord_clarify_buttons.py -q -k 'TestClarifyChoiceViewConstruction'
# 3 passed

scripts/run_tests.sh \
  tests/gateway/test_discord_prompt_timeout_config.py \
  tests/tools/test_clarify_gateway.py \
  tests/gateway/test_clarify_send_timeout_ambiguity.py -q
# 47 passed

Live verification on Discord/Linux:

  • agent.clarify_timeout: 0
  • resolved clarify view timeout: None
  • resolved execution-approval view timeout: 300
  • gateway restarted onto the patched checkout successfully.

Checklist

  • Read CONTRIBUTING.md
  • Searched open and closed issues/PRs
  • Cross-referenced the overlapping open PR instead of claiming no prior work
  • Branch contains only the four scoped code/test files
  • Added red-capable regression tests
  • Ran focused canonical test commands
  • Preserved bounded execution-approval behavior

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/gateway Gateway runner, session dispatch, delivery comp/plugins Plugin system and bundled plugins platform/discord Discord bot adapter sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 29, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference; please use your judgment.

Overall: Aligns the Discord clarify button view's lifetime with the gateway's actual clarify wait. ClarifyChoiceView now uses _read_discord_clarify_timeout() (derived from the canonical get_clarify_timeout(), None when unlimited → View(timeout=None)) instead of the approval-specific approvals.discord_prompt_timeout, and the gateway's _clarify_send_then_wait returns a distinct cancellation sentinel for unlimited timeouts instead of a misleading "did not respond within 0m". Tests cover both sides.

What it does

  • _read_discord_clarify_timeout: None when the canonical timeout ≤ 0 (unlimited), else the seconds; 3600.0 fallback on any exception. Security-sensitive approval views keep the separately bounded approvals.discord_prompt_timeout.
  • _clarify_send_then_wait: when timeout <= 0, returns "[clarify wait cancelled before user response]" — avoids the nonsensical "within 0m" message on cancellation.
  • Tests: unlimited clarify keeps Discord buttons live (view.timeout is None); unlimited-wait cancellation returns the new sentinel.

Non-blocking notes

  • The bare except Exception in _read_discord_clarify_timeout silently falls back to 3600s. If tools.clarify_gateway.get_clarify_timeout is unavailable for a reason other than absence (import/config error), the view lifetime (3600s) can diverge from the configured agent.clarify_timeout. A logger.debug in the except branch would make such a divergence diagnosable.
  • Behavior change worth confirming: with unlimited clarify, the Discord view now stays interactive with no expiry until the gateway resolves the prompt. That's the intended fix, but a stale card left on screen stays clickable indefinitely — verify the existing interaction/cleanup path still stop()s the view (and disables buttons) once the answer arrives.

Non-blocking — please use your judgment.

@sake303

sake303 commented Sep 7, 2026

Copy link
Copy Markdown

I added concrete lifecycle reproductions to the existing issue: #97662 (comment). I am consolidating my overlapping #104916 there rather than keeping another tracker open.

This also supports the earlier review's note about retiring resolved views. Against main source at d8a07768c5ee59afae62e9fb51d45e1e30aa74da, an offline test using the real discord.py ViewStore timer reproduced:

  • An accepted clarify answer later overwritten with Prompt expired — no action taken.
  • A missing clarify entry still rendered as Answered despite failed resolution.
  • The completed-to-expired overwrite on execution-approval cards through the shared view lifecycle.

Would you prefer to include terminal-view cleanup and render-after-acceptance handling in this PR, or keep them as a coordinated follow-up under #97662? The issue comment includes pinned source anchors, test setup and limits, and proposed regression cases. Pending execution approvals should retain their separately bounded timeout.

This evidence is against the pinned main source, not a test of this PR's head. It supports the existing cleanup acceptance criteria without proposing a different timeout policy.

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

Labels

comp/gateway Gateway runner, session dispatch, delivery comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have platform/discord Discord bot adapter 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 sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants