Skip to content

fix(relay): keep clarify choices readable - #93154

Open
tw0316 wants to merge 2 commits into
NousResearch:mainfrom
tw0316:fix/relay-clarify-readable-options
Open

tw0316 wants to merge 2 commits into
NousResearch:mainfrom
tw0316:fix/relay-clarify-readable-options

Conversation

@tw0316

@tw0316 tw0316 commented Aug 23, 2026

Copy link
Copy Markdown

Summary

  • render complete numbered clarify choices in relay prompt content
  • keep prompt controls compact (Choose 1, Choose 2, …) while preserving positional callback mapping
  • fall back to bounded multi-message text when the negotiated platform limit cannot fit the prompt, preserving typed-answer capture

Why

Slack and other clients can visually truncate descriptive button labels well below their API character caps. The relay path currently puts the full choice only in the button label, so users cannot reliably distinguish choices.

This supplements the native Slack fix in #77547 without touching the same files. Refs #78115.

Verification

  • pytest -q tests/gateway/relay/test_relay_interactive.py
  • pytest -q tests/gateway/relay
  • pytest -q tests/gateway -k clarify
  • ruff check gateway/relay/adapter.py tests/gateway/relay/test_relay_interactive.py
  • git diff --check

@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Aug 23, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

Solid approach: moving the full choice text into the prompt body and keeping button labels positional ("Choose 1/2") fixes the truncation/readability problem without breaking the callback-safe c{i} id scheme, and the oversize fallback with _interim_send correctly avoids sealing open drafts (good test for that regression). A few points worth a look:

  1. Multi-select hint lost in the oversize fallbackgateway/relay/adapter.py:2863 hardcodes "Reply with the number, the option text, or your own answer." The base-class text fallback (gateway/platforms/base.py, default send_clarify) looks up the pending entry's multi_select flag and, when set, tells the user to reply with comma/space-separated numbers. Since this new fallback also flips the entry into awaiting_text mode where _selection_attempt_tokens accepts token lists (tools/clarify_gateway.py:227-246), a multi-select clarify that happens to exceed the platform limit would now receive single-select instructions. Suggest mirroring the base-class lookup (_cg._entries.get(clarify_id).multi_select) or factoring the hint construction into a shared helper so the two paths can't drift further — they already disagree on wording.

  2. mark_awaiting_text result ignoredgateway/relay/adapter.py:2865. If the registry entry is missing at send time (id mismatch, or registration raced), the call silently no-ops and the user's free-text reply falls through to native-button classification, where prose is rejected rather than captured (tools/clarify_gateway.py:342-367). The base class shares this pattern, but since this is a brand-new call site, a debug/warning log when it returns False would make this failure mode diagnosable.

  3. Chunk-send loop duplicationgateway/relay/adapter.py:2871-2880: copy metadata, tag _interim_send, loop chunks, early-return on first failure. This mirrors existing interim-send patterns in this file; extracting a small _send_chunked_interim(chat_id, text, metadata) helper would keep the flag semantics in one place as more call sites appear.

Minor: gateway/relay/adapter.py:2846-2849 could use enumerate(choice_labels) instead of range(len(...)); purely stylistic.

Tests cover both the happy path with an oversized choice and the draft-sealing edge case — nice coverage of the tricky interaction.

@tw0316

tw0316 commented Aug 23, 2026

Copy link
Copy Markdown
Author

Addressed the substantive multi-select concern in 9fd36daff: oversized text fallback now mirrors the base adapter's multi-select instructions, with a regression proving comma/space-separated selection guidance remains visible. Full relay suite: 245 passed.

I left the other suggestions unchanged intentionally:

  • mark_awaiting_text() return handling matches the existing base-adapter contract; adding logging only in this override would make equivalent paths inconsistent.
  • The interim chunk loop remains local because this is currently one narrow call site; extracting a helper now would add abstraction without reuse.
  • The range(len(...)) loop makes the positional callback index explicit and is stylistic only.

@kvnloo

kvnloo commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Exact-head pick-one cross-link on #93154 head 9fd36da.

CHECK vs KEEP #111093 head 32d76fb for clarify choices on relay. Close as superseded by #111093.

Abort if head drifts.

1 similar comment
@kvnloo

kvnloo commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Exact-head pick-one cross-link on #93154 head 9fd36da.

CHECK vs KEEP #111093 head 32d76fb for clarify choices on relay. Close as superseded by #111093.

Abort if head drifts.

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 P2 Medium — degraded but workaround exists sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants