Skip to content

feat(channels/yuanbao): add Yuanbao channel provider#2600

Merged
senamakel merged 15 commits into
tinyhumansai:mainfrom
senamakel:pr/2494-fixes
May 25, 2026
Merged

feat(channels/yuanbao): add Yuanbao channel provider#2600
senamakel merged 15 commits into
tinyhumansai:mainfrom
senamakel:pr/2494-fixes

Conversation

@senamakel
Copy link
Copy Markdown
Member

@senamakel senamakel commented May 25, 2026

Summary

  • New Yuanbao channel provider (Tencent 元宝 robot logic v5) under src/openhuman/channels/providers/yuanbao/, wired into the channel registry + startup loop and exposed to the UI through the existing ChannelSetupModal flow.
  • Outbound: HMAC-SHA256 sign-token client (sign.rs) with cached tokens, message splitting (splitter.rs), and a COS uploader (cos.rs) for media.
  • Inbound: framed protobuf WebSocket client (connection.rs + wire.rs + proto_biz.rs) with bounded reconnect, single-flight token refresh on code=10099, prompt shutdown response, and bounded length-delimited parsing.
  • Frontend: YuanbaoConfig.tsx form (i18n-keyed validation messages), YuanbaoIcon, and ChannelSetupModal integration for icon parity with ChannelSelector.
  • Backend storage hygiene: app_secret is verified at connect time via the live sign-token endpoint, persisted only in the encrypted credentials store (never in plaintext config.toml), and hydrated at startup with a stale-app_key guard.

Problem

Yuanbao was not a supported channel; users on Tencent Yuanbao bots had no first-class way to wire OpenHuman to their robot.

Solution

This PR supersedes #2494 (from YuanbaoTeam/openhuman) because that PR did not have "Allow edits from maintainers" enabled and we could not push fixes directly. This version includes the original implementation plus:

  1. Merge conflict resolution with current main (25 i18n chunk files).
  2. Security fix: replaced 14 unconditional console.log/warn/error calls in YuanbaoConfig.tsx that leaked credential key names in production devtools — now uses the namespaced debug('channels:yuanbao') logger.
  3. i18n fix: dedicated channels.yuanbao.{connect,reconnect,savedRestartRequired} keys instead of borrowing from channels.telegram.*.
  4. Stability fix: replaced expect() panic in ops.rs with proper error propagation via ok_or_else()?.
  5. Correctness fix: use definition.id instead of definition.icon for Yuanbao detection in ChannelSetupModal.
  6. Build fix: fixed duplicate keys in de-5.ts causing TS1117 compile errors.
  7. CodeRabbit fixes: re-entrancy guard, graceful shutdown cleanup, reconnect budget reset, varint overflow rejection.

Submission Checklist

If a section does not apply to this change, mark the item as N/A with a one-line reason. Do not delete items.

  • Tests added or updated (happy path + at least one failure / edge case) per Testing Strategy
  • Diff coverage ≥ 80% — changed lines (Vitest + cargo-llvm-cov merged via diff-cover) meet the gate enforced by .github/workflows/coverage.yml. Run pnpm test:coverage and pnpm test:rust locally; PRs below 80% on changed lines will not merge.
  • Coverage matrix updated — added/removed/renamed feature rows in docs/TEST-COVERAGE-MATRIX.md reflect this change (or N/A: behaviour-only change)
  • All affected feature IDs from the matrix are listed in the PR description under ## Related
  • No new external network dependencies introduced (mock backend used per Testing Strategy)
  • N/A: this PR does not touch release-cut surfaces (installer / code signing / OS permission prompts / DMG / Gatekeeper / accessibility)
  • N/A: no prior GitHub issue; this PR introduces the channel directly. Supersedes feat(channels/yuanbao): add Yuanbao channel provider #2494.

Impact

  • Desktop only. No Tauri shell changes; the channel runs entirely in the in-process Rust core. Added Rust deps were already present in the repo (reqwest, tungstenite, hmac/sha2). No web / mobile / CLI behavior changes outside the channel surface.
  • Security: app_secret never lands in plaintext config.toml; runtime loads it from the encrypted credentials store at startup. Stale-app_key profiles will no longer be paired with the new key.
  • Performance: sign-token cache + single-flight refresh; bounded WS backoff with prompt-exit on shutdown. No new periodic timers beyond the existing channel supervisor.

Related

Summary by CodeRabbit

Release Notes

  • New Features

    • Added Yuanbao channel integration with full setup and configuration support.
    • New dedicated channel icon and UI components for Yuanbao.
    • Added multilingual support for Yuanbao across 15 languages.
  • Tests

    • Added comprehensive test coverage for channel configuration and operations.

Review Change Stack

lrt4836 and others added 10 commits May 25, 2026 10:07
Adds a new Yuanbao channel provider so OpenHuman can talk to the
Yuanbao bot service over signed WebSocket + HTTPS.

- Backend: full provider under `src/openhuman/channels/providers/yuanbao/`
  (signed WS connection, inbound/outbound pipelines, COS media upload,
  protobuf wire codec, ID shortening for conversation-store filenames),
  wired through channel config schema, registry, runtime startup,
  controllers, and CLI.
- Frontend: dedicated `YuanbaoConfig` form + icon, hooked into the
  channel selector, setup modal, and connection slice using the
  existing per-channel definition pattern.
- Endpoint config: prod / pre-release defaults selectable via `env`
  field; no test credentials in code.
Resolve all 9 actionable comments from the CodeRabbit review.

Frontend:
- ChannelSetupModal: render branded YuanbaoIcon for the yuanbao channel
  instead of falling through CHANNEL_ICONS to an empty emoji.
- YuanbaoConfig: route required-field validation and the connecting
  spinner copy through i18n; branch on connectChannel's reported status
  so non-"connected" results surface as errors instead of being silently
  treated as success.
- i18n: add 3 new keys (fieldRequired / connecting / unexpectedStatus)
  to en, ko, and all chunk-3 files (zh-CN translated; other locales use
  English placeholders to keep `pnpm i18n:check` green).

Backend (Rust):
- ops.rs: stop persisting `app_secret` in plaintext config.toml — the
  encrypted credentials store already holds it. Also persist the optional
  endpoint overrides (env / api_domain / ws_domain / route_env) so a
  non-default cluster selection survives restart.
- startup.rs: extract `resolve_yuanbao_app_secret` and load the secret
  from the credentials store at startup when TOML is empty. Pre-existing
  TOML values still win so manually-installed deployments don't break.
- channel.rs: drop the hex preview of inbound biz payloads from the
  pipeline-error log path; user content / PII must not leak to logs.
- connection.rs: re-check `*shutdown.borrow()` after connect_once
  returns. Without this, a shutdown signal observed inside connect_once
  consumes the `changed()` notification, leaving the outer
  `tokio::select!` to wait through the full reconnect backoff before
  exiting.
- proto_biz.rs: replace `as i32` / `as u32` casts on decoded varints
  with `varint_to_i32` / `varint_to_u32` helpers backed by `try_from`,
  so oversized fields surface a structured `ProtoDecode` error instead
  of silently truncating `code`, `member_count`, `role`, `join_time`,
  and `next_offset`.
- wire.rs: guard the `WT_LEN` arithmetic with `usize::try_from(len)` and
  `pos.checked_add(...)`, eliminating the slice-panic path on
  adversarial length-delimited fields.

Tests (+6 net):
- proto_biz: `decode_biz_rsp_code_rejects_varint_out_of_i32_range`,
  `decode_group_member_list_rejects_varint_out_of_u32_range`.
- wire: `parse_fields_oversize_len_field_errors_without_panic`.
- ops: updated `connect_yuanbao_persists_when_credentials_valid` to
  assert TOML has no plaintext secret and the store has both fields;
  new `connect_yuanbao_persists_env_override` covering the endpoint
  round-trip.
- startup: three tests around `resolve_yuanbao_app_secret` (load from
  credentials, prefer existing TOML, gracefully return empty).
- connection: `run_exits_promptly_after_shutdown_signal` regression
  guard against backoff-blocked shutdown.

cargo test --lib -- yuanbao: 188 passed (was 182).
pnpm i18n:check: green.
…humansai#2494

Three CodeRabbit-flagged issues from the follow-up review:

1) ops.rs — apply endpoint overrides BEFORE preflight verification.

   The previous flow rebuilt YuanbaoConfig from `config.channels_config.yuanbao`
   alone inside `verify_yuanbao_credentials`, so client-supplied
   `env` / `api_domain` / `ws_domain` / `route_env` overrides were ignored
   at verify-time but applied at persistence-time. A user submitting
   `env = "pre"` could pass verification against PROD's sign-token
   cluster and then fail auth after restart when the persisted override
   took effect.

   The verifier now consumes a pre-built effective `YuanbaoConfig`. A new
   `build_effective_yuanbao_config` helper overlays the overrides on top
   of the existing TOML, calls `apply_env_defaults`, and produces the
   single config used for BOTH verify and persistence — they can no
   longer diverge.

2) cos.rs — tighten `get_cos_credentials_sends_route_env_header_when_non_empty`.

   The wiremock matcher only checked for `X-Route-Env: canary`, so a
   future refactor routing the call to the wrong endpoint would still
   pass the test as long as some POST carried that header. Bind the
   matcher to `UPLOAD_INFO_PATH` as well.

3) startup.rs — don't hydrate `app_secret` from a stale profile.

   `resolve_yuanbao_app_secret` used to copy whatever secret was in the
   encrypted store, even if `yb_cfg.app_key` had been edited in
   `config.toml`. That would silently pair a new key with an old secret
   on next startup and the channel would fail auth until the user
   reconnected manually.

   Now compare the stored profile's `app_key` metadata to `yb_cfg.app_key`
   before copying. On mismatch, log a warning naming both keys and leave
   `app_secret` empty so `YuanbaoChannel::new`'s `validate()` step fails
   loudly instead of attempting auth with a stale value.

Tests added:
- ops_tests: `connect_yuanbao_verifies_against_overridden_api_domain`
  proves the verifier hits the override URI even when base TOML
  `api_domain` points at a black hole (`http://127.0.0.1:1`), and that
  the same override is the one persisted.
- startup tests: `skips_hydration_when_stored_profile_has_different_app_key`
  proves we leave `app_secret` empty when the store profile is keyed
  to a different `app_key`.

cargo test -- yuanbao: 190 passed, 0 failed.
Per the matrix update contract in `docs/TEST-COVERAGE-MATRIX.md` — any
PR that adds, removes, or changes a feature leaf must update the
matrix in the same change. This PR introduces the Yuanbao channel
provider, so add the leaf row pointing at the RU test paths landed in
this PR (sign-token preflight, credentials store hydration including
the stale-app_key guard, WS reconnect/shutdown).

Status 🟡 not ✅: there is no dedicated WDIO spec for Yuanbao yet —
the connect-flow UI is rendered via the generic `ChannelSetupModal`
that is already covered by other channel flow specs.
The Coverage Gate (diff-cover ≥ 80%) job was failing on PR tinyhumansai#2494
at 13% because the yuanbao channel surface lacked Vitest coverage.
Add targeted unit tests that lift diff coverage on the touched
files well above the 80% threshold:

- YuanbaoIcon: 33% → 100% (default/custom className, unique
  clipPath ids per instance for collision-free dual rendering)
- YuanbaoConfig: 2% → 95.1% (renders fields, returns null with
  no auth modes, inline validation + clear-on-input, all four
  connect outcomes — connected/restart_required/restart-fails/
  non-connected/connect-throws — disconnect success+failure,
  stale-connecting reset on mount, lastError rendering)
- ChannelSetupModal: 60% → 100% (yuanbao SVG branch + yuanbao
  switch case + emoji branch + fallback message + Escape close)
- channelConnectionsSlice: 87.5% → 100% (ensureChannelModes
  lazy-init when persisted state is missing the yuanbao key)

Diff-cover total on changed lines: 13% → 84%.
Upstream refactored `getChannelIcons()` in `skills/skillIcons.tsx` to
take a `t()` translator and read each entry's aria-label from
`skills.channelIcon.<channel>`. Add the matching `yuanbao` entry to
`en.ts` and all 13 locale chunks so the yuanbao branch (resolved
during rebase onto upstream/main) does not break `pnpm i18n:check`.

zh-CN translated as 元宝; other locales carry the same `Yuanbao`
placeholder as the existing un-translated `discord`/`telegram` keys.
# Conflicts:
#	app/src/lib/i18n/chunks/ar-3.ts
#	app/src/lib/i18n/chunks/ar-5.ts
#	app/src/lib/i18n/chunks/bn-3.ts
#	app/src/lib/i18n/chunks/bn-5.ts
#	app/src/lib/i18n/chunks/de-3.ts
#	app/src/lib/i18n/chunks/de-5.ts
#	app/src/lib/i18n/chunks/es-3.ts
#	app/src/lib/i18n/chunks/es-5.ts
#	app/src/lib/i18n/chunks/fr-3.ts
#	app/src/lib/i18n/chunks/fr-5.ts
#	app/src/lib/i18n/chunks/hi-3.ts
#	app/src/lib/i18n/chunks/hi-5.ts
#	app/src/lib/i18n/chunks/id-3.ts
#	app/src/lib/i18n/chunks/id-5.ts
#	app/src/lib/i18n/chunks/it-3.ts
#	app/src/lib/i18n/chunks/it-5.ts
#	app/src/lib/i18n/chunks/ko-3.ts
#	app/src/lib/i18n/chunks/ko-5.ts
#	app/src/lib/i18n/chunks/pt-3.ts
#	app/src/lib/i18n/chunks/pt-5.ts
#	app/src/lib/i18n/chunks/ru-3.ts
#	app/src/lib/i18n/chunks/ru-5.ts
#	app/src/lib/i18n/chunks/zh-CN-3.ts
#	app/src/lib/i18n/chunks/zh-CN-5.ts
#	app/src/lib/i18n/ko.ts
…s, remove expect() panic

- Replace 14 unconditional console.log/warn/error calls in YuanbaoConfig.tsx
  with the namespaced debug() logger to avoid leaking credential key names
  in production devtools.
- Use dedicated channels.yuanbao.{connect,reconnect,savedRestartRequired}
  i18n keys instead of borrowing from channels.telegram.*.
- Add the 3 new keys to en.ts, en-3.ts, and all 12 locale -3.ts chunks.
- Replace expect() with ok_or_else()? in ops.rs to avoid panicking the
  core process if prebuilt_yuanbao_config is unexpectedly None.
- Use definition.id instead of definition.icon for the yuanbao check in
  ChannelSetupModal to avoid breakage if icon name diverges from id.
- Remove duplicate de-5.ts keys that caused TS1117 errors.
@senamakel senamakel requested a review from a team May 25, 2026 05:44
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 25, 2026

Caution

Review failed

Failed to post review comments

📝 Walkthrough

Walkthrough

Adds a complete Yuanbao channel provider: frontend UI (icons, ChannelSetupModal, YuanbaoConfig), Redux/store updates, i18n strings, extensive Rust backend provider (config, SignManager, ops verification, WebSocket connection, inbound pipeline, outbound sender), protocol codecs, media/COS upload support, tests, and module wiring.

Changes

Yuanbao Channel Integration

Layer / File(s) Summary
Channel type and backend definition
src/openhuman/channels/controllers/definitions.rs, app/src/types/channels.ts
Register yuanbao channel definition and add 'yuanbao' to ChannelType.
Frontend icons & selector
app/src/components/channels/YuanbaoIcon.tsx, app/src/components/channels/ChannelSelector.tsx, app/src/components/skills/skillIcons.tsx
Add YuanbaoIcon SVG component, use helper to render yuanbao icon, and expose in skill icons.
Channel setup UI
app/src/components/channels/ChannelSetupModal.tsx, app/src/components/channels/YuanbaoConfig.tsx
Integrate YuanbaoConfig into modal; implement config UI with field validation, connect/disconnect flows, restart handling, and status badges.
Frontend tests
app/src/components/channels/__tests__/*
Add Vitest+RTL tests for ChannelSetupModal routing, YuanbaoConfig workflows, and YuanbaoIcon uniqueness/DOM id handling.
Store & i18n
app/src/store/channelConnectionsSlice.ts, app/src/lib/i18n/chunks/*, app/src/lib/i18n/en.ts
Defensive per-channel bucket initialization, migration additions for new channels, and add Yuanbao UI strings across language chunks.
Backend config & startup
src/openhuman/channels/providers/yuanbao/config.rs, src/openhuman/config/schema/channels.rs, src/openhuman/channels/runtime/startup.rs
Add YuanbaoConfig struct, defaults/validation, wire startup to hydrate app_secret from credentials store and initialize channel.
Credential verification & ops tests
src/openhuman/channels/providers/yuanbao/sign.rs, src/openhuman/channels/controllers/ops.rs, src/openhuman/channels/controllers/ops_tests.rs
Implement SignManager (HMAC-SHA256 signing + caching/coalescing), verify sign-token during connect_channel, persist config without plaintext secret, add wiremock tests.
Connection transport
src/openhuman/channels/providers/yuanbao/connection.rs
Implement YuanbaoConnection: auth-bind handshake, ping heartbeats, pending correlator/send_and_wait, reconnection/backoff, shutdown semantics, and tests.
Message pipeline & channel facade
src/openhuman/channels/providers/yuanbao/inbound.rs, src/openhuman/channels/providers/yuanbao/outbound.rs, src/openhuman/channels/providers/yuanbao/channel.rs
17-stage inbound middleware, OutboundSender APIs (text/image/file/upload via COS), YuanbaoChannel wiring including aliasing, heartbeat management, and message splitting.
Protocol codecs & utilities
src/openhuman/channels/providers/yuanbao/wire.rs, proto.rs, proto_biz.rs, proto_constants.rs, types.rs, media.rs, cos.rs, ids.rs, splitter.rs, errors.rs, Cargo.toml
Add protobuf wire primitives (safe varints), ConnMsg/TIM codecs, biz-layer codecs, protocol constants, image parsing, COS HMAC-SHA1 signing/upload, ID shortening, markdown splitter, error enums, and add sha1 = "0.10".
Module organization & health check
src/openhuman/channels/providers/mod.rs, src/openhuman/channels/mod.rs, src/openhuman/channels/commands.rs, docs/TEST-COVERAGE-MATRIX.md
Expose pub mod yuanbao, re-export YuanbaoChannel/YuanbaoConfig, integrate into doctor_channels health-checks, and add test-coverage matrix entry.

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Suggested labels

agent

Suggested reviewers

  • graycyrus

🐰 A whisker-twitch of joy! The Yuanbao gold now flows
Through pipes of TypeScript, Rust, and proto-codes
Where tokens sign with HMAC-256 pride
And messages split fence-aware, far and wide
The garden grows—another channel blooms! 🌿✨

@coderabbitai coderabbitai Bot added feature Net-new user-facing capability or product behavior. working A PR that is being worked on by the team. labels May 25, 2026
@senamakel senamakel self-assigned this May 25, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

🧹 Nitpick comments (5)
src/openhuman/channels/providers/yuanbao/config.rs (1)

143-161: 💤 Low value

Consider documenting the expected call order for apply_env_defaults and validate.

validate() requires ws_domain and api_domain (when no token) to be non-empty, but apply_env_defaults() fills these from env. If a caller validates before applying defaults, validation fails unexpectedly. The tests show the intended pattern (defaults applied first), but the API doesn't enforce or document it.

A defensive option would be to call apply_env_defaults internally at the start of validate, or add a doc comment clarifying the expected order.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/channels/providers/yuanbao/config.rs` around lines 143 - 161,
The validate() method on the Yuanbao config currently fails if env-derived
fields (ws_domain, api_domain when token is absent) haven't been populated;
update validate to defensively call apply_env_defaults() at the start (or ensure
it returns/propagates any error) so env defaults are applied before checks, then
perform the existing checks and return YuanbaoError::Config as before;
alternatively, if you prefer not to mutate state during validation, add a clear
doc comment on validate mentioning callers must call apply_env_defaults() first
and reference the apply_env_defaults and validate functions in the docstring.
src/openhuman/channels/controllers/ops.rs (1)

180-194: 💤 Low value

Minor: Consider reusing an existing HTTP client.

verify_yuanbao_credentials creates a new reqwest::Client on each call. While acceptable for a one-time connect operation, the existing pattern elsewhere in the codebase typically shares a client for connection pooling benefits.

This is fine for now since credential verification is infrequent.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/channels/controllers/ops.rs` around lines 180 - 194,
verify_yuanbao_credentials creates a fresh reqwest::Client for each call which
prevents reuse of connection pooling; change the function signature to accept a
shared reqwest::Client (or obtain the existing shared client) and pass that into
SignManager::new instead of constructing reqwest::Client::new() so
verify_yuanbao_credentials uses the shared client instance used elsewhere in the
codebase (refer to verify_yuanbao_credentials and SignManager::new(...) in the
diff).
src/openhuman/channels/providers/yuanbao/sign.rs (1)

261-261: 💤 Low value

Consider explicit handling when code field is missing.

At line 261, if the response JSON lacks a code field, it defaults to 0, which means the code proceeds as if successful. The subsequent check for data at line 263-270 provides a safety net, but an explicit check for a missing code field would make the intent clearer and fail faster with a more descriptive error.

This is a minor robustness improvement since the current flow still fails correctly.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/channels/providers/yuanbao/sign.rs` at line 261, The current
extraction let code = json.get("code").and_then(|c| c.as_i64()).unwrap_or(0)
masks a missing "code" field by treating it as success; change the logic in the
handler (where variable code is created) to explicitly check whether
json.get("code") is Some and return a clear Err (or Err variant/early return)
when it's absent or not an integer, rather than falling back to 0, so callers of
this function (the response parsing around code and data) get a descriptive
failure instead of proceeding as if successful.
app/src/components/channels/__tests__/YuanbaoConfig.test.tsx (1)

82-95: ⚡ Quick win

Avoid className-coupled validation assertions.

This assertion depends on text-coral, so harmless style refactors can break the test. Assert the validation message count/content directly instead of CSS classes.

As per coding guidelines: “Prefer testing behavior over implementation details.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/src/components/channels/__tests__/YuanbaoConfig.test.tsx` around lines 82
- 95, The test currently filters elements by className ('text-coral') to detect
validation errors; update the assertions to avoid coupling to CSS by checking
the validation text/content directly: use screen.getAllByText(/AppID/) or
screen.queryAllByText(/AppID/) and assert on the returned array length (e.g.,
toBeGreaterThan(0) before input and toHaveLength(0) or toBe(0) after firing
fireEvent.change on the '元宝开放平台 AppID' field) and keep the expect that
channelConnectionsApi.connectChannel was not called; locate and modify the
assertions around screen.getAllByText, screen.queryAllByText, and the
fireEvent.change usage in YuanbaoConfig.test.tsx.
src/openhuman/channels/providers/yuanbao/splitter.rs (1)

6-7: 💤 Low value

Documentation mentions table-row handling but implementation doesn't track table state.

The module doc mentions "a Markdown table row (lines starting with |)" but safe_to_break only checks fence state. If table preservation was intentional, the state tracking is missing; otherwise, consider updating the doc comment.

Also applies to: 81-83

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/channels/providers/yuanbao/splitter.rs` around lines 6 - 7, The
module doc claims splitter handles "a Markdown table row (lines starting with
`|`)" but the implementation (see function safe_to_break and surrounding state)
never tracks table state; either remove/update that sentence in the module
comment or implement table detection: add a boolean field (e.g., in the same
state struct used by safe_to_break) that is set when a line begins with '|' (and
cleared on blank lines or other block boundaries) and then update safe_to_break
to return false while in-table so table rows are preserved; pick one approach
and ensure you reference/modify the state struct and the safe_to_break function
accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@app/src/components/channels/YuanbaoConfig.tsx`:
- Around line 67-93: Add a synchronous re-entrancy guard to prevent rapid
repeated clicks from enqueueing duplicate actions: at the very start of
handleConnect check the local busy flag and return immediately if busy; do the
same for the other action handler (the disconnect/restart handler around lines
186–208). Keep the existing setBusy(true) usage but ensure the early busy check
is present before any async work or dispatch (and ensure setBusy(false) is
restored on completion/error so the guard is cleared).

In `@src/openhuman/channels/providers/yuanbao/channel.rs`:
- Around line 94-100: PipelineState is being created with config.bot_id at
startup so when config.bot_id is empty the echo-guard (SkipSelfMw) cannot match
self-sent messages after sign-token/auth-bind resolves the canonical bot id;
locate where sign-token/auth-bind yields the resolved bot id and update the
inbound pipeline with it by either calling a setter on the existing
PipelineState (e.g., PipelineState::set_from_account or set_bot_id) or by
replacing the pipeline with a new InboundPipeline(PipelineState::new(&config,
resolved_bot_id)); ensure the same PipelineState instance used by
InboundPipeline is updated (or pipeline swapped) after auth so SkipSelfMw can
match outbound echoes.
- Around line 294-296: After sending the shutdown signal via shutdown_tx, do not
immediately abort conn_task; instead await the connection task to allow
YuanbaoConnection::shutdown() to run and perform its cleanup so sender, pending,
and is_connected are updated; replace conn_task.abort() with awaiting the task's
JoinHandle (optionally with a timeout) and handle its Result, ensuring listen()
returns only after the task finishes.

In `@src/openhuman/channels/providers/yuanbao/connection.rs`:
- Around line 207-243: The reconnect attempt counter is never reset so
intermittent clean disconnects will exhaust max_attempts; after a successful
session (when self.connect_once(...) returns Ok(...) in the match) reset attempt
to 0 instead of only incrementing later. Locate the match on outcome in the
connection loop (connect_once, outcome, NO_RECONNECT_CLOSE_CODES) and set
attempt = 0 in the Ok arms (except where you return for no-reconnect close
codes) so cleanly closed/authenticated sessions restore the reconnect budget
before the backoff and subsequent sleep logic.

In `@src/openhuman/channels/providers/yuanbao/outbound.rs`:
- Around line 23-27: The send_* functions (e.g., send_group_message and
send_c2c_message) currently treat any frame with a matching msg_id as success;
after calling send_and_wait() they must inspect the business-layer response
payload (resp.data) and verify the inner response code/message before returning
success. Fix: after send_and_wait() in send_group_message and send_c2c_message,
decode or parse resp.data to extract the business fields (e.g., code and
message) using the appropriate decoder for that response (or parse JSON/proto
from resp.data), and if the inner code != 0 return/propagate an error containing
that code/message instead of success; apply the same check to the
heartbeat/send_*_heartbeat paths mentioned. Ensure you reference and use
send_and_wait, send_group_message, send_c2c_message (and the heartbeat send
functions) so the logic always validates resp.data.code before signaling
success.

In `@src/openhuman/channels/providers/yuanbao/wire.rs`:
- Around line 42-55: The varint decoder loop in wire.rs currently allows a
malformed 10th-byte varint to overflow/truncate; update the loop in the varint
decoding function (the block using variables i, pos, shift, byte, value and
returning YuanbaoError::ProtoDecode) to reject a 10th byte with any payload bits
other than the low bit: specifically, when shift == 63 (i.e. handling the 10th
byte) check if (byte & 0xFE) != 0 and return
Err(YuanbaoError::ProtoDecode("varint too long".into())) instead of shifting and
accepting those bits; keep the existing checks for truncated inputs and shift >=
64.

---

Nitpick comments:
In `@app/src/components/channels/__tests__/YuanbaoConfig.test.tsx`:
- Around line 82-95: The test currently filters elements by className
('text-coral') to detect validation errors; update the assertions to avoid
coupling to CSS by checking the validation text/content directly: use
screen.getAllByText(/AppID/) or screen.queryAllByText(/AppID/) and assert on the
returned array length (e.g., toBeGreaterThan(0) before input and toHaveLength(0)
or toBe(0) after firing fireEvent.change on the '元宝开放平台 AppID' field) and keep
the expect that channelConnectionsApi.connectChannel was not called; locate and
modify the assertions around screen.getAllByText, screen.queryAllByText, and the
fireEvent.change usage in YuanbaoConfig.test.tsx.

In `@src/openhuman/channels/controllers/ops.rs`:
- Around line 180-194: verify_yuanbao_credentials creates a fresh
reqwest::Client for each call which prevents reuse of connection pooling; change
the function signature to accept a shared reqwest::Client (or obtain the
existing shared client) and pass that into SignManager::new instead of
constructing reqwest::Client::new() so verify_yuanbao_credentials uses the
shared client instance used elsewhere in the codebase (refer to
verify_yuanbao_credentials and SignManager::new(...) in the diff).

In `@src/openhuman/channels/providers/yuanbao/config.rs`:
- Around line 143-161: The validate() method on the Yuanbao config currently
fails if env-derived fields (ws_domain, api_domain when token is absent) haven't
been populated; update validate to defensively call apply_env_defaults() at the
start (or ensure it returns/propagates any error) so env defaults are applied
before checks, then perform the existing checks and return YuanbaoError::Config
as before; alternatively, if you prefer not to mutate state during validation,
add a clear doc comment on validate mentioning callers must call
apply_env_defaults() first and reference the apply_env_defaults and validate
functions in the docstring.

In `@src/openhuman/channels/providers/yuanbao/sign.rs`:
- Line 261: The current extraction let code = json.get("code").and_then(|c|
c.as_i64()).unwrap_or(0) masks a missing "code" field by treating it as success;
change the logic in the handler (where variable code is created) to explicitly
check whether json.get("code") is Some and return a clear Err (or Err
variant/early return) when it's absent or not an integer, rather than falling
back to 0, so callers of this function (the response parsing around code and
data) get a descriptive failure instead of proceeding as if successful.

In `@src/openhuman/channels/providers/yuanbao/splitter.rs`:
- Around line 6-7: The module doc claims splitter handles "a Markdown table row
(lines starting with `|`)" but the implementation (see function safe_to_break
and surrounding state) never tracks table state; either remove/update that
sentence in the module comment or implement table detection: add a boolean field
(e.g., in the same state struct used by safe_to_break) that is set when a line
begins with '|' (and cleared on blank lines or other block boundaries) and then
update safe_to_break to return false while in-table so table rows are preserved;
pick one approach and ensure you reference/modify the state struct and the
safe_to_break function accordingly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 29cac06e-2dc3-404c-8717-90802e523b0f

📥 Commits

Reviewing files that changed from the base of the PR and between cc67177 and aa8fb8e.

⛔ Files ignored due to path filters (2)
  • Cargo.lock is excluded by !**/*.lock
  • app/src-tauri/Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (65)
  • Cargo.toml
  • app/src/components/channels/ChannelSelector.tsx
  • app/src/components/channels/ChannelSetupModal.tsx
  • app/src/components/channels/YuanbaoConfig.tsx
  • app/src/components/channels/YuanbaoIcon.tsx
  • app/src/components/channels/__tests__/ChannelSetupModal.test.tsx
  • app/src/components/channels/__tests__/YuanbaoConfig.test.tsx
  • app/src/components/channels/__tests__/YuanbaoIcon.test.tsx
  • app/src/components/skills/skillIcons.tsx
  • app/src/lib/i18n/chunks/ar-3.ts
  • app/src/lib/i18n/chunks/ar-5.ts
  • app/src/lib/i18n/chunks/bn-3.ts
  • app/src/lib/i18n/chunks/bn-5.ts
  • app/src/lib/i18n/chunks/de-3.ts
  • app/src/lib/i18n/chunks/de-5.ts
  • app/src/lib/i18n/chunks/en-3.ts
  • app/src/lib/i18n/chunks/en-5.ts
  • app/src/lib/i18n/chunks/es-3.ts
  • app/src/lib/i18n/chunks/es-5.ts
  • app/src/lib/i18n/chunks/fr-3.ts
  • app/src/lib/i18n/chunks/fr-5.ts
  • app/src/lib/i18n/chunks/hi-3.ts
  • app/src/lib/i18n/chunks/hi-5.ts
  • app/src/lib/i18n/chunks/id-3.ts
  • app/src/lib/i18n/chunks/id-5.ts
  • app/src/lib/i18n/chunks/it-3.ts
  • app/src/lib/i18n/chunks/it-5.ts
  • app/src/lib/i18n/chunks/ko-3.ts
  • app/src/lib/i18n/chunks/ko-5.ts
  • app/src/lib/i18n/chunks/pt-3.ts
  • app/src/lib/i18n/chunks/pt-5.ts
  • app/src/lib/i18n/chunks/ru-3.ts
  • app/src/lib/i18n/chunks/ru-5.ts
  • app/src/lib/i18n/chunks/zh-CN-3.ts
  • app/src/lib/i18n/chunks/zh-CN-5.ts
  • app/src/lib/i18n/en.ts
  • app/src/store/__tests__/channelConnectionsSlice.test.ts
  • app/src/store/channelConnectionsSlice.ts
  • app/src/types/channels.ts
  • docs/TEST-COVERAGE-MATRIX.md
  • src/openhuman/channels/commands.rs
  • src/openhuman/channels/controllers/definitions.rs
  • src/openhuman/channels/controllers/ops.rs
  • src/openhuman/channels/controllers/ops_tests.rs
  • src/openhuman/channels/mod.rs
  • src/openhuman/channels/providers/mod.rs
  • src/openhuman/channels/providers/yuanbao/channel.rs
  • src/openhuman/channels/providers/yuanbao/config.rs
  • src/openhuman/channels/providers/yuanbao/connection.rs
  • src/openhuman/channels/providers/yuanbao/cos.rs
  • src/openhuman/channels/providers/yuanbao/errors.rs
  • src/openhuman/channels/providers/yuanbao/ids.rs
  • src/openhuman/channels/providers/yuanbao/inbound.rs
  • src/openhuman/channels/providers/yuanbao/media.rs
  • src/openhuman/channels/providers/yuanbao/mod.rs
  • src/openhuman/channels/providers/yuanbao/outbound.rs
  • src/openhuman/channels/providers/yuanbao/proto.rs
  • src/openhuman/channels/providers/yuanbao/proto_biz.rs
  • src/openhuman/channels/providers/yuanbao/proto_constants.rs
  • src/openhuman/channels/providers/yuanbao/sign.rs
  • src/openhuman/channels/providers/yuanbao/splitter.rs
  • src/openhuman/channels/providers/yuanbao/types.rs
  • src/openhuman/channels/providers/yuanbao/wire.rs
  • src/openhuman/channels/runtime/startup.rs
  • src/openhuman/config/schema/channels.rs

Comment thread app/src/components/channels/YuanbaoConfig.tsx
Comment thread src/openhuman/channels/providers/yuanbao/channel.rs
Comment thread src/openhuman/channels/providers/yuanbao/channel.rs
Comment thread src/openhuman/channels/providers/yuanbao/connection.rs
Comment thread src/openhuman/channels/providers/yuanbao/outbound.rs
Comment thread src/openhuman/channels/providers/yuanbao/wire.rs
coderabbitai[bot]
coderabbitai Bot previously approved these changes May 25, 2026
…own, reconnect, varint

- Add synchronous `if (busy) return` guard in both handleConnect and
  handleDisconnect to prevent duplicate calls from rapid double-clicks.
- Give conn_task a 2s timeout to run its own shutdown cleanup before
  force-aborting, so sender/pending/is_connected state stays consistent.
- Reset reconnect attempt counter after a successful session so
  intermittent disconnects don't permanently exhaust the budget.
- Reject invalid 10-byte varints (10th byte > 1) instead of silently
  truncating, preventing malformed frames from decoding to wrong values.
senamakel added 4 commits May 24, 2026 23:23
…ct resolution

The 3 keys (customGifError, customGifHeading, customGifLabel) at their
original position (~line 243) were mistakenly removed as duplicates during
merge conflict resolution. They are the canonical copies on main; the
later occurrence (from a different merge state) was the duplicate. Restore
the originals and remove the actual duplicates so CI's merge-with-main
produces the correct key count.
# Conflicts:
#	src/openhuman/channels/controllers/ops_tests.rs
The 10th-byte overflow check (added for CodeRabbit) fires before the
"too long" guard when all bytes are 0x80. Update the test assertion to
accept either error message.
@coderabbitai coderabbitai Bot added the agent Built-in agents, prompts, orchestration, and agent runtime in src/openhuman/agent/. label May 25, 2026
@senamakel senamakel merged commit d997394 into tinyhumansai:main May 25, 2026
31 of 37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent Built-in agents, prompts, orchestration, and agent runtime in src/openhuman/agent/. feature Net-new user-facing capability or product behavior. working A PR that is being worked on by the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants