Skip to content

feat(google-chat): render clarify prompts as cards - #36068

Closed
aeyeopsdev wants to merge 2 commits into
NousResearch:mainfrom
AeyeOps:feat/google-chat-clarify-cards
Closed

feat(google-chat): render clarify prompts as cards#36068
aeyeopsdev wants to merge 2 commits into
NousResearch:mainfrom
AeyeOps:feat/google-chat-clarify-cards

Conversation

@aeyeopsdev

@aeyeopsdev aeyeopsdev commented May 31, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Renders Google Chat clarify() prompts with choices as Chat cards with buttons, while preserving the existing text fallback path.

This makes Google Chat clarify prompts easier to answer from the Chat UI without changing the underlying clarify tool contract.

Related Google Chat PRs: #36027, #36035, #36061. Card-click callback handling is intentionally split into a follow-up PR.

Related Issue

N/A — focused Google Chat UX improvement for existing gateway clarify prompts.

Type of Change

  • ✨ New feature (non-breaking change that adds functionality)
  • ✅ Tests (adding or improving test coverage)

Changes Made

  • plugins/platforms/google_chat/adapter.py: add a small Cards v2 renderer for constrained card specs.
  • plugins/platforms/google_chat/adapter.py: add send_card() for posting cardsV2 payloads with thread metadata.
  • plugins/platforms/google_chat/adapter.py: override send_clarify() to render choice prompts as Google Chat cards and preserve text-response fallback.
  • tests/gateway/test_google_chat.py: cover card rendering, threaded send_card(), and clarify card payloads.

How to Test

  1. python -m py_compile plugins/platforms/google_chat/adapter.py tests/gateway/test_google_chat.py
  2. git diff --check
  3. scripts/run_tests.sh tests/gateway/test_google_chat.py
  4. python /root/.codex/skills/hermes-upstream-pr/scripts/pr_preflight.py --base upstream/main

Checklist

Code

  • I've read the Contributing Guide.
  • My commit message follows Conventional Commits.
  • I searched for existing PRs to make sure this isn't a duplicate.
  • My PR contains only changes related to this feature slice.
  • I've added tests for my changes.
  • I've tested on Linux.

Documentation & Housekeeping

  • Documentation update N/A.
  • cli-config.yaml.example update N/A.
  • CONTRIBUTING.md / AGENTS.md update N/A.
  • Cross-platform impact considered: adapter payload construction and tests only.
  • Tool descriptions/schemas update N/A.

Related PRs

Screenshots / Logs

Validation passed locally:

scripts/run_tests.sh tests/gateway/test_google_chat.py
163 tests passed, 0 failed

@mxnstrexgl mxnstrexgl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — automated review passed. No security, quality, or test coverage issues detected.

@aeyeopsdev
aeyeopsdev force-pushed the feat/google-chat-clarify-cards branch from d612a8b to fcab9a0 Compare June 30, 2026 12:20
@aeyeopsdev
aeyeopsdev marked this pull request as ready for review June 30, 2026 13:00
mark_awaiting_text is the 'Other (type answer)' mode-flip, not a send-time
setup call — invoking it in send_clarify forces the user's next message to
be captured as the clarify response, racing the button-click path and
bypassing the buttons entirely. Telegram calls it only in the 'other'
callback branch; do the same here.
@aeyeopsdev
aeyeopsdev force-pushed the feat/google-chat-clarify-cards branch from 5065b0c to 8f60aa1 Compare July 5, 2026 23:27

@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 isolating the Cards v2 renderer and preserving the typed-reply path. This slice is not operational on its own, however.

Problems

  • plugins/platforms/google_chat/adapter.py:2067 emits hermes_clarify, but current main's Pub/Sub handler explicitly ACKs and discards card/widget events at plugins/platforms/google_chat/adapter.py:1211-1217. Thus a click cannot resolve the pending clarify, despite the adapter contract requiring button taps to reach tools.clarify_gateway.resolve_gateway_clarify (gateway/platforms/ADDING_A_PLATFORM.md:119). The PR body correctly identifies #36069 as the missing callback work.
  • _clarify_state is written at plugins/platforms/google_chat/adapter.py:2105, but this PR has no reader or cleanup path. Successful cards accumulate state until restart.

Suggested changes

  • Salvage this together with a tested inbound card-click resolver, including the Other text-entry transition.
  • Add an inbound-event test that verifies a card click resolves the clarify and removes its state.

Automated hermes-sweeper review.

buttons.append(
{
"text": label,
"action": "hermes_clarify",

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 action has no receiver in this PR: current main ACKs and discards Google Chat card/widget events at plugins/platforms/google_chat/adapter.py:1211-1217. Please land it with inbound dispatch to resolve_gateway_clarify, or retain the text renderer until that callback path is available.

)
result = await self.send_card(chat_id, card, metadata=metadata)
if result.success:
self._clarify_state[clarify_id] = session_key

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 state is never read or cleared by this PR, so every successful clarify card leaves an entry until adapter restart. Move it with the callback implementation or add lifecycle cleanup and an inbound-resolution test.

@teknium1 teknium1 added 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:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Merged via #65546 (rebase-merged onto current main, head commit fce298f) — both your commits were cherry-picked with your authorship preserved in git log. Thanks for the contribution!

@teknium1 teknium1 closed this Jul 16, 2026
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 sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants