Skip to content

feat(gateway): streaming & markdown support for Google Chat adapter#743

Merged
thepagent merged 9 commits intoopenabdev:mainfrom
canyugs:feat/gateway-googlechat-streaming-markdown
May 6, 2026
Merged

feat(gateway): streaming & markdown support for Google Chat adapter#743
thepagent merged 9 commits intoopenabdev:mainfrom
canyugs:feat/gateway-googlechat-streaming-markdown

Conversation

@canyugs
Copy link
Copy Markdown
Contributor

@canyugs canyugs commented May 5, 2026

Summary

Adds streaming (edit_message) and markdown formatting to the Google Chat adapter, implementing the OAB gateway streaming protocol.

User sends message
  │
  ▼
Gateway → OAB core (GatewayEvent)
  │
  ▼  core starts streaming
OAB core sends reply with request_id
  │
  ▼
Gateway sends message → Google Chat API
  │  returns message_name
  ▼
Gateway sends GatewayResponse { message_id } → core
  │
  ▼  core streams tokens
OAB core sends edit_message commands (reply_to = message_id)
  │
  ▼
Gateway PATCH /v1/{message_name}?updateMask=text → updates message in-place

Changes

File Change Description
gateway/src/adapters/googlechat.rs MOD edit_message(): PATCH message text in-place for streaming. handle_reply(): accepts event_tx, routes edit_message command, sends GatewayResponse with message_id (single-chunk, multi-chunk, and token-failure paths). markdown_to_gchat(): converts markdown bold/italic/strikethrough/links/code/headings to Google Chat native formatting. api_base field for test injection.
gateway/src/main.rs MOD Pass event_tx to gc.handle_reply() for streaming support
docs/google-chat.md MOD Move markdown from "Not Supported" to supported, add streaming

Key Design Decisions

  1. OAB gateway streaming protocol — Implements the standard request_id → GatewayResponse → edit_message flow. Core sends initial reply with request_id, gateway returns GatewayResponse with message_id, core sends subsequent edit_message commands to update the message in-place.

  2. PATCH for edit_message — Google Chat REST API documents PATCH /v1/{message.name}?updateMask=text for message updates. Verified working in E2E testing.

  3. GatewayResponse on all paths — Single-chunk, multi-chunk, and token-failure paths all send GatewayResponse back to core. Prevents core from waiting 5s for a response that never arrives (token-failure ack follows the same pattern as Feishu adapter).

  4. markdown_to_gchat conversion**bold***bold*, ~~strike~~~strike~, [text](url)<url|text>, # Heading*bold heading*, code blocks preserved. Unsupported syntax (tables) passes through unchanged. Table rendering is tracked separately in feat(markdown): per-channel table rendering mode (follow-up to #513) #514.

  5. Injectable api_baseGoogleChatAdapter carries an api_base field (defaults to https://chat.googleapis.com/v1). Tests inject a wiremock server URL so they can assert on both success and failure responses without hitting real APIs.

Testing

E2E tested on k3s VPS with Helm chart deployment:

Scenario Result
Streaming short reply (1 edit_message) PASS
Streaming long reply (40+ edit_messages, typewriter effect) PASS
Streaming markdown reply (bold, code blocks) PASS
Non-streaming multi-chunk (>4096 chars, streaming=false) PASS
Markdown formatting (bold, links, code) renders in Google Chat PASS
cargo test — 112 gateway tests passed (47 googlechat) PASS

New test cases (wiremock-based)

Test Verifies
handle_reply_sends_gateway_response_success GatewayResponse with message_id on successful send
handle_reply_sends_failure_response_on_api_error GatewayResponse with success=false on API 500
handle_reply_token_failure_sends_error_response Failure ack when no credentials configured
handle_reply_edit_message_does_not_send_response edit_message does not produce GatewayResponse
handle_reply_multi_chunk_sends_gateway_response GatewayResponse with first chunk's message_id

Not Yet Supported

Breaking Changes

None. Existing non-streaming Google Chat messages continue to work.

Discord Discussion URL

https://discord.com/channels/1491295327620169908/1501153334042824764

🤖 Generated with Claude Code

canyugs and others added 5 commits May 5, 2026 16:03
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…mand

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Multi-chunk path now sends GatewayResponse (prevents core timeout on long messages)
- Token failure sends failure GatewayResponse (parity with Feishu adapter)
- edit_message uses PATCH instead of PUT (per Google Chat API docs)
- Inject api_base for testability
- Rewrite integration tests with wiremock (hermetic, no real API calls)
- Update docs/google-chat.md: move markdown to supported, add streaming

Addresses #2

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
~~text~~ → ~text~ (Google Chat native strikethrough syntax)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 5, 2026 14:48
@canyugs canyugs requested a review from thepagent as a code owner May 5, 2026 14:48
@github-actions github-actions Bot added pending-screening PR awaiting automated screening closing-soon PR missing Discord Discussion URL — will auto-close in 3 days labels May 5, 2026
@github-actions github-actions Bot removed the closing-soon PR missing Discord Discussion URL — will auto-close in 3 days label May 5, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR enhances the gateway’s Google Chat adapter to support the OpenAB gateway streaming protocol by returning a message_id for the initial send and handling subsequent edit_message updates, while also adding Markdown-to-Google-Chat formatting conversion.

Changes:

  • Add streaming support via edit_message (PATCH update) and GatewayResponse { message_id } acknowledgements for initial sends.
  • Add markdown_to_gchat() conversion and apply it to both send and edit operations.
  • Update docs to reflect streaming/markdown support and update the gateway entrypoint to pass event_tx into the adapter.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File Description
gateway/src/main.rs Passes event_tx into the Google Chat adapter reply handler to enable streaming acknowledgements.
gateway/src/adapters/googlechat.rs Implements edit-in-place streaming, returns GatewayResponse with message_id, adds Markdown conversion, and introduces injectable api_base for tests.
docs/google-chat.md Documents streaming and markdown support for the Google Chat adapter.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread gateway/src/adapters/googlechat.rs Outdated
Comment on lines +297 to +301
let text = &reply.content.text;
let chunks = split_text(text, GOOGLE_CHAT_MESSAGE_LIMIT);

for chunk in chunks {
send_message(
if chunks.len() <= 1 {
let msg_name = send_message(
Comment thread gateway/src/adapters/googlechat.rs Outdated
Comment on lines +315 to +319
request_id: req_id.clone(),
success: msg_name.is_some(),
thread_id: None,
message_id: msg_name,
error: if chunks.is_empty() {
Comment thread gateway/src/adapters/googlechat.rs Outdated
Comment on lines +346 to +352
let resp = crate::schema::GatewayResponse {
schema: "openab.gateway.response.v1".into(),
request_id: req_id.clone(),
success: first_msg_name.is_some(),
thread_id: None,
message_id: first_msg_name,
error: None,
Comment on lines +665 to +669
// Bold: **text** → *text*
if chars[i] == '*' && i + 1 < chars.len() && chars[i + 1] == '*' {
if let Some(end) = find_closing(&chars, i + 2, &['*', '*']) {
out.push('*');
let inner: String = chars[i + 2..end].iter().collect();
Comment thread docs/google-chat.md Outdated
- **Bot message filtering** — bot messages (`user_type: "BOT"`) are filtered at the gateway level
- **Message splitting** — long replies (>4096 chars) are automatically split at newline/space boundaries
- **Token auto-refresh** — service account JWT tokens are refreshed automatically before expiry
- **Markdown formatting** — replies are converted via `markdown_to_gchat` to Google Chat's native formatting (`*bold*`, `_italic_`, `` `code` ``, code blocks, `<url|text>` links); unsupported syntax (strikethrough, headings beyond bold) is approximated
@shaun-agent
Copy link
Copy Markdown
Contributor

OpenAB PR Screening

This is auto-generated by the OpenAB project-screening flow for context collection and reviewer handoff.
Click 👍 if you find this useful. Human review will be done within 24 hours. We appreciate your support and contribution 🙏

Screening report ## Intent

PR #743 is trying to make the Google Chat gateway adapter support in-place streaming replies and native Google Chat markdown formatting.

The user-visible problem is that Google Chat users currently do not get the same streaming response experience available in other adapters, and markdown emitted by OpenAB/core does not render cleanly in Google Chat. The operator-visible problem is that the gateway needs to complete the existing request_id -> GatewayResponse -> edit_message contract so core can stream reliably without timing out waiting for a message id.

Feat

This is a feature PR.

It adds two main behaviors:

  • Google Chat replies can be updated in place via edit_message, using Google Chat PATCH /v1/{message.name}?updateMask=text.
  • Common markdown is converted into Google Chat’s supported formatting syntax before delivery.

It also updates gateway wiring so the Google Chat adapter can send GatewayResponse events back to core, and it updates docs to list streaming and markdown as supported.

Who It Serves

Primary beneficiaries:

  • Google Chat end users, who get streamed responses and better formatted messages.
  • Agent runtime operators, because the adapter participates in the gateway streaming protocol instead of leaving core waiting for response acks.
  • Maintainers and reviewers, because the PR adds test injection via api_base and wiremock coverage for success and failure paths.

Rewritten Prompt

Implement Google Chat adapter support for OpenAB gateway streaming.

When Google Chat receives a normal reply with a request_id, send the initial message to Google Chat, capture the returned message.name, and emit a GatewayResponse to core containing a stable message_id that later edit_message commands can target. For edit_message, update the existing Google Chat message in place with PATCH /v1/{message.name}?updateMask=text.

Also add a focused markdown conversion layer for Google Chat formatting: bold, italic, strikethrough, links, inline code, code blocks, and headings. Leave unsupported markdown unchanged.

Requirements:

  • Preserve existing non-streaming Google Chat behavior.
  • Send GatewayResponse on success and failure paths where core expects an ack.
  • Do not send GatewayResponse for edit_message commands.
  • Add tests for successful send, API failure, missing-token failure, edit-message routing, and multi-chunk response behavior.
  • Document streaming and markdown support in docs/google-chat.md.

Merge Pitch

This is worth advancing because it brings Google Chat closer to parity with other gateway adapters and implements a core protocol expectation rather than a cosmetic feature. Streaming support improves perceived latency for users, while GatewayResponse coverage reduces timeout-prone behavior between gateway and core.

Risk profile is moderate. The behavioral surface is adapter-local, but the PR touches message routing and ack semantics, which are easy to get subtly wrong. The likely reviewer concern is whether the chosen message_id is stable and sufficient for future edit routing, especially across single-chunk, multi-chunk, and failure paths.

Best-Practice Comparison

Relevant OpenClaw principles:

  • Explicit delivery routing is directly relevant. The adapter must map core’s logical message_id to Google Chat’s message.name clearly enough for later edits.
  • Retry/backoff and run logs are partially relevant. This PR appears to handle immediate success/failure, but does not add durable retry behavior.
  • Isolated executions are not central here because this is adapter message handling, not scheduled job execution.
  • Durable job persistence is not directly relevant unless streaming state needs to survive gateway restarts.
  • Gateway-owned scheduling is not relevant.

Relevant Hermes Agent principles:

  • Atomic writes for persisted state are not relevant unless the adapter starts persisting message mappings.
  • Fresh session per scheduled run and self-contained prompts are not relevant.
  • Gateway daemon tick model is not relevant.
  • File locking to prevent overlap is not directly relevant, though the spirit applies if concurrent edit streams can race on the same Google Chat message.

The strongest comparison point is routing correctness: OpenClaw’s explicit delivery routing principle supports making the message_id -> Google Chat message.name contract easy to reason about, test, and maintain.

Implementation Options

Option 1: Conservative adapter-local merge

Accept the current approach if tests pass and the implementation keeps all streaming and markdown logic isolated to googlechat.rs. Require reviewer attention on message_id semantics, error ack behavior, and markdown edge cases.

Option 2: Balanced protocol-aligned cleanup

Merge the feature after tightening the adapter contract: define helper functions for converting Google Chat API message names into gateway message ids, centralize GatewayResponse construction, and ensure edit routing is explicit and documented in tests. Keep persistence out of scope.

Option 3: Ambitious cross-adapter streaming hardening

Use this PR as the trigger to formalize gateway streaming behavior across adapters. Add shared helper types for GatewayResponse, edit routing, ack failure handling, and adapter test fixtures. Google Chat becomes one implementation of a clearer cross-adapter contract.

Comparison Table

Option Speed to ship Complexity Reliability Maintainability User impact Fit for OpenAB right now
Conservative adapter-local merge High Low Medium Medium High for Google Chat users Good if reviewer confidence is high
Balanced protocol-aligned cleanup Medium Medium High High High for Google Chat users Best fit
Cross-adapter streaming hardening Low High Very high long-term Very high long-term Broader future impact Too large for this PR unless split

Recommendation

Advance PR #743 using the balanced option.

The feature itself is valuable and scoped to a real parity gap, but the next review should focus on protocol correctness rather than just whether Google Chat streaming works in E2E testing. Before merge, tighten or confirm the message id mapping, GatewayResponse behavior on every expected path, and markdown conversion boundaries.

Keep table rendering, file attachments, and space events out of this PR. If maintainers want broader streaming consistency work, split that into a follow-up after this adapter-specific feature lands.

canyugs added a commit to canyugs/openab that referenced this pull request May 5, 2026
…bdev#743

5 review items from #3:

1. Empty message: short-circuit to skip API call, send failure ack
2. Single-chunk send failure: propagate error string (status + body) in GatewayResponse.error
3. Multi-chunk send failure: propagate first error string instead of error: None
4. Italic: convert *text* → _text_ (Google Chat italic syntax). Single _text_ passes through.
5. docs/google-chat.md: align with actual converter behavior (bold, italic, strikethrough, headings)

Refactor: send_message now returns Result<String, String> so error context flows to core.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…bdev#743

5 review items from #3:

1. Empty message: short-circuit to skip API call, send failure ack
2. Single-chunk send failure: propagate error string (status + body) in GatewayResponse.error
3. Multi-chunk send failure: propagate first error string instead of error: None
4. Italic: convert *text* → _text_ (Google Chat italic syntax). Single _text_ passes through.
5. docs/google-chat.md: align with actual converter behavior (bold, italic, strikethrough, headings)

Refactor: send_message now returns Result<String, String> so error context flows to core.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@canyugs canyugs force-pushed the feat/gateway-googlechat-streaming-markdown branch from 46b9a0b to b7abe0d Compare May 5, 2026 16:34
@canyugs
Copy link
Copy Markdown
Contributor Author

canyugs commented May 5, 2026

Addressed all 5 Copilot review comments in b7abe0d:

# Fix
1 Empty message: short-circuit, skip API call, send failure ack with error: "empty message"
2 Single-chunk send failure: propagate status + body to GatewayResponse.error (refactored send_message to return Result<String, String>)
3 Multi-chunk failure: propagate first encountered error string instead of error: None
4 Italic: *text*_text_ (Google Chat italic syntax). Single _text_ passes through. Whitespace-adjacent asterisks not matched (avoids matching 2 * 3 * 4)
5 docs/google-chat.md: rewrote markdown bullet to enumerate actual converter behavior (bold, italic, strikethrough, headings, links, code)

New tests added (119 total, 0 failed):

  • markdown_italic_asterisk / markdown_italic_does_not_match_bold / markdown_italic_underscore_passes_through / markdown_italic_no_match_when_unbalanced
  • handle_reply_empty_message_short_circuits (asserts expect(0) API calls)
  • handle_reply_multi_chunk_failure_includes_error
  • Updated handle_reply_sends_failure_response_on_api_error to assert error contains status code

E2E tested image available at ghcr.io/canyugs/openab-gateway:pr-743 for anyone who wants to verify on a Google Chat workspace.

@chaodu-agent
Copy link
Copy Markdown
Collaborator

chaodu-agent commented May 5, 2026

CHANGES REQUESTED 🟡 — Solid implementation of the OAB streaming protocol for Google Chat, with well-tested markdown conversion. Address the NITs below, then ready to merge.

📋 Review — Four Questions

1. What problem does this solve?

Google Chat adapter lacked streaming (typewriter effect) and sent raw markdown that rendered poorly. This PR adds:

  • edit_message support via PATCH API for in-place message updates during streaming
  • markdown_to_gchat() converter for Google Chat's native formatting syntax
  • Proper GatewayResponse flow so OAB core knows the message_id for subsequent edits

2. How does it solve it?

  • Streaming: handle_reply now accepts event_tx, routes edit_message commands to a new edit_message() method that PATCHes the message. Initial reply sends GatewayResponse with message_id back to core.
  • Markdown: markdown_to_gchat() converts **bold***bold*, *italic*_italic_, ~~strike~~~strike~, [text](url)<url|text>, headings → bold. Code blocks pass through unchanged.
  • Error handling: All paths (single-chunk, multi-chunk, token-failure, empty message) send GatewayResponse — prevents core from timing out waiting for ack.
  • Testability: api_base field on adapter allows wiremock injection.
  • send_message returns Result<String, String> — propagates message_name on success, error context on failure.

3. What alternatives were considered?

  • Google Chat Cards API for rich formatting — more complex, not needed for text streaming
  • PUT for edit — PATCH with updateMask=text is the documented approach

4. Is this the best approach?

Yes. Clean implementation following the established OAB streaming protocol pattern (same as Feishu/Discord).

🟢 INFO — Well done:

  • Comprehensive wiremock-based tests (5 new async tests covering success, failure, empty, multi-chunk, edit_message)
  • Token-failure ack pattern matches Feishu adapter (prevents 5s core timeout)
  • Code block detection correctly skips markdown conversion inside fenced blocks
  • Injectable api_base is a good pattern for testability
  • Italic conversion handles the tricky *text* vs **bold** disambiguation correctly

🟡 NIT — Must fix:

  1. markdown_to_gchat is called both in send_message (for normal replies) and in edit_message (for streaming updates). If core sends pre-formatted text for edits, this could double-convert. Worth a comment noting the assumption that core always sends raw markdown.
  2. The convert_inline function uses Vec<char> which allocates. For hot-path streaming with many edit_message calls, this could be optimized with byte-level iteration. Not a concern at current scale but worth noting for future.
  3. Minor: find_closing only handles 2-char patterns. If future syntax needs single-char closing (e.g., single _ for italic), it won't work — but find_single already covers that case, so this is fine.
🔍 Baseline Check (Step 0)
  • Main branch googlechat.rs: no edit_message, no markdown_to_gchat, no api_base field, send_message returns (), handle_reply doesn't take event_tx
  • docs/google-chat.md on main lists "Markdown rendering" under "Not Supported"
  • All 772 additions are net-new value
  • 6 commits show iterative refinement (initial → review feedback → strikethrough → final fixes)

@Joseph19820124
Copy link
Copy Markdown
Contributor

Quick question on the Google Chat adapter setup: is a Google Workspace (enterprise) account required to create a Google Chat bot and use the service account auth flow documented here?

Specifically wondering whether a regular Google account (gmail.com) can create a Google Chat app/bot via Google Cloud Console, or if the bot publishing and service account JWT flow only works for Workspace-managed domains.

This affects how accessible this integration is to individual developers vs. enterprise teams.

- markdown_to_gchat: doc comment noting caller must pass raw markdown
  (called by both send_message and edit_message; double-conversion
  would happen if pre-converted text is passed)
- convert_inline: TODO note for future byte-level iteration optimization
  (currently Vec<char> allocation per line, acceptable at current scale)

Addresses chaodu-agent must-fix #1 and #2 from PR openabdev#743 review.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@canyugs
Copy link
Copy Markdown
Contributor Author

canyugs commented May 6, 2026

Addressing review feedback

@chaodu-agent must-fix items (f766af8)

# Action
1 Added doc comment to markdown_to_gchat noting caller must pass raw markdown (called from both send_message and edit_message). Double-conversion would happen if pre-converted text is passed; OAB core is expected to always emit raw markdown.
2 Added TODO(perf) note above convert_inline for future byte-level iteration optimization. Current Vec<char> allocation is acceptable at expected streaming scale.
3 No-op — already covered by find_single for single-char patterns.

@Joseph19820124 — re Google Workspace requirement

A Google Workspace (Business or Enterprise) account is required. A regular @gmail.com account cannot create a Google Chat bot — Google blocks Chat API configuration for non-Workspace accounts.

Per Google's official setup docs, the prerequisite is verbatim:

"A Business or Enterprise Google Workspace account with access to Google Chat"

Specifics:

  • A personal Gmail can create a GCP project and toggle the Chat API on, but configuration ultimately fails with "Google Chat API is only available to Google Workspace users"
  • Service account creation works mechanically but Chat API calls are blocked at runtime/config since the developer account isn't part of a Workspace tenant
  • Workaround: Workspace Individual / free trial / Business Starter — cheapest tier is sufficient

I can update docs/google-chat.md to call this out in the Prerequisites section if helpful.

Regular @gmail.com consumer accounts cannot create Google Chat apps —
Google requires a Workspace (Business or Enterprise) account at API
configuration. Cheapest qualifying tier is Workspace Individual or
Business Starter.

Per Joseph19820124 question on PR openabdev#743.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@canyugs
Copy link
Copy Markdown
Contributor Author

canyugs commented May 6, 2026

Updated docs/google-chat.md Prerequisites in acd224e to call out the Workspace account requirement explicitly, with link to Google's official setup docs.

@chaodu-agent
Copy link
Copy Markdown
Collaborator

🟢 LGTM — solid implementation following established patterns

Verdict: No blocking issues. This PR correctly implements the OAB gateway streaming protocol for Google Chat, matching the Feishu adapter pattern exactly.

What it does well

  • GatewayResponse on all paths — single-chunk success, multi-chunk, empty message, token failure, and API error all send a response back to core. This prevents the 5-second timeout that would occur if the gateway silently dropped a reply.
  • Streaming via PATCHedit_message uses PATCH /v1/{message.name}?updateMask=text, which is the documented Google Chat API for in-place message updates.
  • Test coverage — wiremock integration tests for success, failure, empty message, multi-chunk, and edit_message paths. Unit tests for all markdown conversion rules.
  • Injectable api_base — enables testing without hitting real APIs. Same pattern as Feishu adapter.
  • Code block preservation — fenced code blocks and inline code are correctly passed through without conversion.
🟡 NITs (non-blocking)
  1. Italic edge casefind_single in the italic detection path could mis-parse nested constructs like *italic **bold** more*. The find_single scan would close the italic at the first * of **bold**. In practice, LLM output rarely produces this nesting, and worst case is slightly garbled formatting (not a crash or data loss). Consider adding a test case to document the known limitation.

  2. convert_inline Vec<char> allocation — acknowledged in the TODO comment. Acceptable at current scale; the streaming hot path calls edit_message frequently but each call processes a single message (≤4096 chars). Not a concern until proven otherwise by profiling.

  3. Multi-chunk partial failure semantics — if the first chunk succeeds but subsequent chunks fail, GatewayResponse reports success: true with the first message_id. This is reasonable (core needs the message_id for streaming edits regardless of later chunk failures), and matches how the Feishu adapter handles multi-message replies. The first_error is still captured and reported.

  4. Docs — the Workspace account prerequisite is a good addition. Minor: could mention that Business Starter has a 14-day free trial for contributors who want to test.

Review methodology
  1. Baseline check — read googlechat.rs on main (webhook handler, JWT verification, plain-text handle_reply, split_text, token cache)
  2. Pattern comparison — verified streaming protocol matches Feishu adapter (edit_message command routing, GatewayResponse on all paths, api_base injection)
  3. Schema verification — confirmed GatewayReply.reply_to carries the message name for edit operations, request_id triggers response flow
  4. Edge case analysis — markdown conversion (code blocks, nested formatting, link parsing with balanced parens), empty message handling, token failure path
  5. Test review — 7 new integration tests (wiremock) + 15 markdown unit tests covering bold, italic, links, headings, code blocks, strikethrough, edge cases

@chaodu-agent
Copy link
Copy Markdown
Collaborator

🔴 One blocking issue found — four-monk collaborative review

Updated verdict: Request changes

The initial LGTM is revised after collaborative triage by 超渡·普渡·覺渡·擺渡. One logic bug found in the multi-chunk path.


🔴 SUGGESTED CHANGES — multi-chunk success logic contradiction

Line 376 in googlechat.rs:

// Current (bug):
success: first_msg_name.is_some(),

// Should be:
success: first_msg_name.is_some() && first_error.is_none(),

Impact: When a long message is split into multiple chunks and chunk 1 succeeds but chunk 2+ fails, the GatewayResponse reports success: true with error: Some(...) — a contradictory signal. Core interprets success: true as "message delivered", but the user only sees a partial message with the rest silently dropped.

Suggested fix: Add && first_error.is_none() so any chunk failure marks the overall operation as failed. Also add a test case for the "mixed success/failure" scenario (e.g., wiremock returning 200 for the first request and 500 for subsequent ones).


🟡 NIT — send_message JSON parse fallback (non-blocking)

Lines 858-859: When the API returns 2xx but the body is not valid JSON, serde_json::from_str(&body).unwrap_or_default() silently produces an empty object. The name field extraction then fails with a generic "missing message name" error, losing the original parse failure context. Consider logging the raw body at warn! level before the fallback.

🟢 INFO — things done well

  • GatewayResponse on all paths — token failure, empty message, API error, and success all send a response. Prevents 5s core timeout.
  • Streaming protocol — correctly implements request_id → GatewayResponse → edit_message flow, matching Feishu adapter pattern.
  • Link parsing safetyparse_md_link uses depth counters for nested [] and (). No panic, no infinite loop, unbalanced brackets gracefully fall back to literal text.
  • send_message error handling — complete coverage of non-2xx, network errors, and missing response fields.
  • Test coverage — wiremock integration tests for success, failure, empty, multi-chunk, edit_message, and token failure paths. 15 markdown unit tests.
  • Code block preservation — fenced and inline code correctly passed through without conversion.
Review methodology (four-monk triage)
  • 超渡 (Kiro): Baseline check on main, pattern comparison with Feishu adapter, schema verification, edge case analysis
  • 普渡 (Claude): Deep code analysis, identified multi-chunk success logic bug, verified link parsing safety
  • 覺渡 (Gemini): Independent verification of all three question points, confirmed multi-chunk bug, validated error handling completeness
  • 擺渡 (Codex): Pending (review in progress)

All three responding monks independently identified the same multi-chunk logic contradiction.

Copy link
Copy Markdown
Collaborator

@chaodu-agent chaodu-agent left a comment

Choose a reason for hiding this comment

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

Found one blocking issue after checking the PR merge ref against current main.

gateway/src/adapters/googlechat.rs adds several GatewayReply test fixtures that construct Content with only content_type and text, for example around handle_reply_sends_gateway_response_success. Current main has Content { content_type, text, attachments }, so the merge result does not compile: these struct literals are missing attachments: Vec::new().

This is easy to miss because the PR head branch still has the older Content shape, but refs/pull/743/merge includes main schema changes. Please add attachments: Vec::new() to the new Google Chat test fixtures, or use a small test helper to construct text content.

I could not run cargo test in this environment because cargo is not installed, but this is a static Rust compile error on the merge ref.

When chunk 1 succeeds but subsequent chunks fail, GatewayResponse was
reporting success=true with error=Some(...) — a contradictory signal.
Core would treat the message as delivered despite missing content.

Now any chunk failure marks the overall operation as failed, while
preserving message_id so core retains the reference for any follow-up.

Adds handle_reply_multi_chunk_partial_failure_reports_failure test
covering the mixed success/failure scenario (wiremock 200→500).

Addresses chaodu-agent blocking review on PR openabdev#743.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@canyugs
Copy link
Copy Markdown
Contributor Author

canyugs commented May 6, 2026

Addressed blocking issue in ca5f8a4:

// Before
success: first_msg_name.is_some(),
// After
success: first_msg_name.is_some() && first_error.is_none(),

Any chunk failure now marks the overall operation as failed. message_id is preserved so core retains the reference.

New test handle_reply_multi_chunk_partial_failure_reports_failure covers the mixed scenario (wiremock 200 → 500) — asserts success=false, message_id=Some(...), error contains the 500 status.

Skipping the non-blocking NITs (italic edge case, JSON parse fallback log, Business Starter trial mention) to keep this PR focused on the streaming/markdown scope.

@chaodu-agent
Copy link
Copy Markdown
Collaborator

🟢 LGTM — Streaming & Markdown for Google Chat

Verdict: Approve-ready. No blocking issues. Well-structured PR that follows the established Feishu adapter streaming pattern.

What this PR does

Adds streaming support (edit_message via PATCH) and markdown-to-Google-Chat formatting to the Google Chat adapter, implementing the full OAB gateway streaming protocol (request_id → GatewayResponse → edit_message).

Traffic Light

Finding
🟢 Correctly implements the gateway streaming protocol — GatewayResponse sent on all paths (success, failure, token-failure, empty message), preventing core 5s timeout
🟢 Multi-chunk partial failure correctly reports success=false while preserving message_id for core reference
🟢 47 new tests with wiremock — hermetic integration tests covering success, failure, empty message, edit_message, multi-chunk, and partial failure scenarios
🟢 api_base injection for testability — clean pattern matching the existing adapter architecture
🟢 send_message returns Result<String, String> — error context flows to GatewayResponse.error
🟡 markdown_to_gchat is a hand-rolled parser while OAB core uses pulldown-cmark in markdown.rs. Acceptable for the limited Google Chat formatting scope, but worth noting for future maintainability
🟡 convert_inline allocates Vec<char> per line (already noted with TODO) — fine at current scale
四問框架 (Four Questions)

1. What problem does it solve?
Google Chat adapter previously only sent static messages with no streaming (typewriter effect) and no markdown formatting. Users saw plain text instead of formatted responses.

2. How does it solve it?

  • edit_message() method: PATCH /v1/{message.name}?updateMask=text for in-place updates
  • markdown_to_gchat(): converts **bold***bold*, *italic*_italic_, ~~strike~~~strike~, [text](url)<url|text>, headings → bold, code blocks pass through
  • handle_reply() accepts event_tx and sends GatewayResponse on all code paths
  • send_message() returns Result<String, String> with message name for edit tracking

3. What alternatives were considered?

  • Follows the same pattern as the Feishu adapter (established precedent)
  • Could have used pulldown-cmark for markdown parsing, but the limited Google Chat formatting surface makes a hand-rolled parser acceptable

4. Is this the best approach?
Yes. The implementation is consistent with the existing Feishu adapter pattern, has comprehensive test coverage, and handles all error paths correctly. The markdown converter scope is appropriately limited to what Google Chat actually supports.

Review notes

Streaming protocol compliance:

  • ✅ Initial reply with request_id → gateway returns GatewayResponse with message_id
  • ✅ Subsequent edit_message commands update in-place via PATCH
  • ✅ Token failure sends failure GatewayResponse (parity with Feishu)
  • ✅ Empty message short-circuits with failure ack

Markdown converter coverage:

  • ✅ Bold (** and __), italic (*), strikethrough (~~)
  • ✅ Headings → bold
  • ✅ Links → <url|text> format
  • ✅ Code blocks and inline code pass through unchanged
  • ✅ Nested formatting in link text
  • ✅ Whitespace-adjacent asterisks not matched as italic (avoids 2 * 3 * 4 false positive)

Test coverage:

  • 47 new tests including wiremock integration tests
  • Covers: success, API error, token failure, empty message, edit_message, multi-chunk, partial failure
  • All tests use injectable api_base — no real API calls

Review by 超渡法師 (chaodu-agent) · Four-monk collaborative review in progress

@thepagent thepagent merged commit ed4fedc into openabdev:main May 6, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pending-contributor pending-screening PR awaiting automated screening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants