feat(channels/yuanbao): add Yuanbao channel provider#2494
Closed
lrt4836 wants to merge 8 commits into
Closed
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a complete Yuanbao (元宝) channel provider to OpenHuman, including frontend UI components, Redux state management, a full WebSocket-based backend provider with protocol codecs, message pipelines, media handling, token signing, and comprehensive tests across 15+ languages. ChangesYuanbao Channel Integration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes This PR introduces a substantial, intricate new channel provider with high-density logic across multiple integration layers: a WebSocket protocol implementation with reconnect, auth, and message correlation; a 17-stage inbound middleware pipeline; media handling with COS integration; token signing with cache coalescing; and comprehensive protocol codecs. The changes span heterogeneous concerns (frontend, state, backend architecture, protocols, operations, tests, i18n) requiring separate reasoning for each layer. The density of new types, state machines, async flows, and protocol-specific logic drives complexity. Suggested reviewers
|
coderabbitai
Bot
added
the
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
app/src/components/channels/YuanbaoConfig.tsx (1)
68-71: ⚡ Quick winUse namespaced
debuginstead ofconsole.*lifecycle logs in this component.Please replace
console.log/warn/errorwith the existingdebug('channels:yuanbao')flow (and keep verbose detail dev-only) to match frontend logging conventions.As per coding guidelines "Follow existing patterns for debug logging (e.g. the
debugnpm package with a namespace per area) plus dev-only detail where useful... Never log secrets, raw JWTs, API keys, or full PII."Also applies to: 82-87, 100-110, 116-128, 142-143, 154-157, 167-168, 177-178
🤖 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/YuanbaoConfig.tsx` around lines 68 - 71, Replace all console.log/console.warn/console.error calls in the YuanbaoConfig component (particularly inside the handleConnect function and other lifecycle/debugging spots) with the project's namespaced debug logger using debug('channels:yuanbao') so logs follow the existing debug flow; ensure messages preserve the same informative text but are emitted via the debug instance and gated for dev-only verbosity, and remove any raw sensitive values (JWTs, API keys, full PII) from logged output before sending to debug. Locate the debug replacements in the YuanbaoConfig component (e.g., the handleConnect function and surrounding blocks where console.* is used) and update each call to use the debug namespace and sanitized message formatting consistent with other components.src/openhuman/channels/providers/yuanbao/connection.rs (1)
451-459: ⚡ Quick winUse
debug/tracefor per-frame diagnostics instead ofinfo.These messages are high-frequency diagnostic logs; keeping them at
infowill create noisy production logs.🔧 Suggested fix
-use tracing::{error, info, warn}; +use tracing::{debug, error, info, warn}; ... - info!( + debug!( "[yuanbao] rx cmd={} module={} cmd_type={} seq={} msg_id={} data_len={}", frame.cmd, frame.module, frame.cmd_type, frame.seq_no, frame.msg_id, frame.data.len() ); ... - info!( + debug!( "[yuanbao] push forwarded to listener cmd={} module={} seq={}", frame.cmd, frame.module, frame.seq_no );As per coding guidelines
src/**/*.rs: “Uselogortracingcrate atdebugortracelevel for Rust diagnostic logs.”Also applies to: 503-506
🤖 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/connection.rs` around lines 451 - 459, The log here uses info! for high-frequency per-frame diagnostics: change the info! macro call that logs "[yuanbao] rx cmd={} module={} cmd_type={} seq={} msg_id={} data_len={}" (which references frame.cmd, frame.module, frame.cmd_type, frame.seq_no, frame.msg_id, frame.data.len()) to debug! (or trace! if you want even lower noise) so these messages are emitted at the appropriate diagnostic level; apply the same change to the similar logging call around the other occurrence that logs the same frame fields (the block noted at lines 503-506) so all per-frame diagnostics use debug/trace instead of info.src/openhuman/channels/providers/yuanbao/channel.rs (1)
268-274: ⚡ Quick winDowngrade per-event diagnostic logs from
infotodebug/trace.These branches are diagnostic/high-frequency paths; logging them at
infowill be noisy in production.As per coding guidelines:
src/**/*.rs: “Uselogortracingcrate atdebugortracelevel for Rust diagnostic logs.”Also applies to: 346-361
🤖 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/channel.rs` around lines 268 - 274, The per-event diagnostic logs inside the Yuanbao channel listen loop should be downgraded from info! to debug! or trace! — locate the listen loop that creates shutdown_rx2 (the tokio::select! block in the channel's listen/loop function) and replace the high-frequency info! calls (e.g., "[yuanbao] channel listening — pipeline ready" and "[yuanbao] listen loop received shutdown") with debug! or trace! as appropriate; apply the same change to the other diagnostic branches referenced (the similar log statements around the other select branches ~346-361) while keeping only true errors or important lifecycle messages at info level.
🤖 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/ChannelSetupModal.tsx`:
- Around line 68-69: The modal header icon is empty because CHANNEL_ICONS lacks
a 'yuanbao' entry so emojiIcon = CHANNEL_ICONS[definition.icon] ?? '' returns
''. Update ChannelSetupModal to handle the yuanbao icon by either adding a
'yuanbao' key to CHANNEL_ICONS mapped to the correct emoji string, or
special-case render YuanbaoIcon when definition.icon === 'yuanbao' (use the
existing YuanbaoIcon component instead of emojiIcon) so the header shows the
correct icon; update both places where emojiIcon is used (including the header
render and the other occurrence around line 92).
In `@app/src/components/channels/YuanbaoConfig.tsx`:
- Line 78: The validation message in YuanbaoConfig uses a hardcoded Chinese
string; replace the literal `${field.label} 不能为空` with the app's i18n call (the
same translate/util used elsewhere in this component) so the message is
localized—e.g. build the message via the translation function with field.label
or a keyed template, and assign that to errors[field.key] instead of the
hardcoded string; update any tests/usage that expect the previous literal if
present.
- Around line 112-117: The handler handleConnect currently marks Yuanbao as
connected unconditionally after calling channelConnectionsApi.connectChannel;
change it to first inspect the returned result.status (from connectChannel) and
only dispatch the connected action when result.status === 'connected', otherwise
handle other statuses (e.g., 'pending' or 'error') by logging the result and
dispatching an appropriate non-connected/failed action or showing UI feedback;
keep references to result, channelConnectionsApi.connectChannel, and the
existing log/console.log calls to aid locating the mutation and ensure no
optimistic connected dispatch occurs before status validation.
In `@src/openhuman/channels/controllers/ops.rs`:
- Around line 405-407: The code is persisting the raw Yuanbao secret by
assigning yb_config.app_secret and saving persisted.channels_config.yuanbao;
instead, stop writing the plaintext secret into persisted config and persist
only a credential reference (e.g., a credential_id or key name). Store the
actual secret in the secure credential store / vault or OS keyring and update
the flow where yb_config is constructed to load the secret from that store at
runtime using the credential reference; remove any assignment to
yb_config.app_secret that would persist the secret and ensure
persisted.channels_config.yuanbao contains only non-sensitive fields and the
credential reference.
- Around line 399-407: When updating the yuanbao config you only set
app_key/app_secret and drop any custom endpoint selection; preserve the client's
chosen endpoint by also copying the env and api_domain into yb_config before
assigning it back to persisted.channels_config.yuanbao. Concretely, read the
existing or incoming endpoint fields (env and api_domain) into yb_config
(alongside app_key and app_secret) so yb_config.env and yb_config.api_domain are
set appropriately before persisted.channels_config.yuanbao = Some(yb_config).
In `@src/openhuman/channels/providers/yuanbao/channel.rs`:
- Around line 408-417: The current pipeline error log computes and emits a hex
preview of the inbound payload (biz_body via preview_len and hex) which may leak
PII; change the warn! call to stop logging raw bytes—remove the hex variable and
its formatting, and instead log only safe metadata such as biz_body.len() and a
redacted marker (e.g. "<redacted>") or a non-reversible digest of the preview
(hash of biz_body[..preview_len]) if you need a correlatable fingerprint; update
the warn! invocation that references preview_len and hex accordingly so no raw
payload bytes are ever written to logs.
In `@src/openhuman/channels/providers/yuanbao/connection.rs`:
- Around line 217-234: The reconnect loop sleeps with backoff even when shutdown
already fired; after the cleanup block (where run() calls self.set_state, clears
sender/pending and increments attempt) detect that shutdown is already signaled
and return immediately instead of awaiting the backoff. Concretely, in the
run()/reconnect loop (the code around connect_once, backoff_seconds, and the
tokio::select) check the shutdown watcher’s current value (e.g. inspect
shutdown.borrow() or use the watch receiver’s immediate-check helper) right
after the cleanup and short-circuit by calling self.shutdown().await; return so
you skip the time::sleep path and avoid delayed shutdown.
In `@src/openhuman/channels/providers/yuanbao/proto_biz.rs`:
- Around line 247-248: The decoder currently narrows get_varint(&fields, ...)
(u64) with as i32/as u32 causing silent truncation; change each cast site (e.g.,
where populating fields like code, member_count, next_offset, role, join_time)
to use i32::try_from(...) or u32::try_from(...), map Err to return
YuanbaoError::ProtoDecode(...) with a clear message, and propagate the error
from the surrounding decoder function instead of using as-casts; locate usages
of get_varint in the decoder functions and replace the direct casts with
try_from conversions and early returns on failure.
In `@src/openhuman/channels/providers/yuanbao/wire.rs`:
- Around line 101-111: The arithmetic that computes end = pos + len as usize can
overflow and panic when a crafted large varint length is returned by
decode_varint; update the decode path in the function using decode_varint so you
perform checked conversions and additions (e.g., convert len to usize safely
with a bounds/try_from or checked_cast and use pos.checked_add(len_usize)) and
validate that pos <= data.len() before slicing, returning
YuanbaoError::ProtoDecode with a clear message on any overflow/invalid length;
ensure the same guarded logic applies to computing remaining bytes
(data.len().checked_sub(pos)) before constructing FieldValue::Bytes and pushing
to out.
---
Nitpick comments:
In `@app/src/components/channels/YuanbaoConfig.tsx`:
- Around line 68-71: Replace all console.log/console.warn/console.error calls in
the YuanbaoConfig component (particularly inside the handleConnect function and
other lifecycle/debugging spots) with the project's namespaced debug logger
using debug('channels:yuanbao') so logs follow the existing debug flow; ensure
messages preserve the same informative text but are emitted via the debug
instance and gated for dev-only verbosity, and remove any raw sensitive values
(JWTs, API keys, full PII) from logged output before sending to debug. Locate
the debug replacements in the YuanbaoConfig component (e.g., the handleConnect
function and surrounding blocks where console.* is used) and update each call to
use the debug namespace and sanitized message formatting consistent with other
components.
In `@src/openhuman/channels/providers/yuanbao/channel.rs`:
- Around line 268-274: The per-event diagnostic logs inside the Yuanbao channel
listen loop should be downgraded from info! to debug! or trace! — locate the
listen loop that creates shutdown_rx2 (the tokio::select! block in the channel's
listen/loop function) and replace the high-frequency info! calls (e.g.,
"[yuanbao] channel listening — pipeline ready" and "[yuanbao] listen loop
received shutdown") with debug! or trace! as appropriate; apply the same change
to the other diagnostic branches referenced (the similar log statements around
the other select branches ~346-361) while keeping only true errors or important
lifecycle messages at info level.
In `@src/openhuman/channels/providers/yuanbao/connection.rs`:
- Around line 451-459: The log here uses info! for high-frequency per-frame
diagnostics: change the info! macro call that logs "[yuanbao] rx cmd={}
module={} cmd_type={} seq={} msg_id={} data_len={}" (which references frame.cmd,
frame.module, frame.cmd_type, frame.seq_no, frame.msg_id, frame.data.len()) to
debug! (or trace! if you want even lower noise) so these messages are emitted at
the appropriate diagnostic level; apply the same change to the similar logging
call around the other occurrence that logs the same frame fields (the block
noted at lines 503-506) so all per-frame diagnostics use debug/trace instead of
info.
🪄 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: ac2efc8b-d851-4e20-8009-d0b91d05d476
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockapp/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
Cargo.tomlapp/src/components/channels/ChannelSelector.tsxapp/src/components/channels/ChannelSetupModal.tsxapp/src/components/channels/YuanbaoConfig.tsxapp/src/components/channels/YuanbaoIcon.tsxapp/src/components/skills/skillIcons.tsxapp/src/store/channelConnectionsSlice.tsapp/src/types/channels.tssrc/openhuman/channels/commands.rssrc/openhuman/channels/controllers/definitions.rssrc/openhuman/channels/controllers/ops.rssrc/openhuman/channels/controllers/ops_tests.rssrc/openhuman/channels/mod.rssrc/openhuman/channels/providers/mod.rssrc/openhuman/channels/providers/yuanbao/channel.rssrc/openhuman/channels/providers/yuanbao/config.rssrc/openhuman/channels/providers/yuanbao/connection.rssrc/openhuman/channels/providers/yuanbao/cos.rssrc/openhuman/channels/providers/yuanbao/errors.rssrc/openhuman/channels/providers/yuanbao/ids.rssrc/openhuman/channels/providers/yuanbao/inbound.rssrc/openhuman/channels/providers/yuanbao/media.rssrc/openhuman/channels/providers/yuanbao/mod.rssrc/openhuman/channels/providers/yuanbao/outbound.rssrc/openhuman/channels/providers/yuanbao/proto.rssrc/openhuman/channels/providers/yuanbao/proto_biz.rssrc/openhuman/channels/providers/yuanbao/proto_constants.rssrc/openhuman/channels/providers/yuanbao/sign.rssrc/openhuman/channels/providers/yuanbao/splitter.rssrc/openhuman/channels/providers/yuanbao/types.rssrc/openhuman/channels/providers/yuanbao/wire.rssrc/openhuman/channels/runtime/startup.rssrc/openhuman/config/schema/channels.rs
lrt4836
added a commit
to YuanbaoTeam/openhuman
that referenced
this pull request
May 23, 2026
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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/channels/providers/yuanbao/media.rs (1)
206-215:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize the response MIME type before returning it.
is_image()andimage_format_code()both compare MIME values case-sensitively. If a server returns a valid header likeImage/PNG, this path preserves that casing and downstream code will treat it as a non-image / unknown format.💡 Suggested fix
let ct = resp .headers() .get(reqwest::header::CONTENT_TYPE) .and_then(|v| v.to_str().ok()) .unwrap_or("") .split(';') .next() .unwrap_or("") .trim() + .to_ascii_lowercase() .to_string();🤖 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/media.rs` around lines 206 - 215, The extracted Content-Type string stored in ct should be normalized (e.g., to lowercase and trimmed) before being returned so downstream checks like is_image() and image_format_code()—which perform case-sensitive comparisons—see a canonical MIME type; update the ct construction (the block that reads headers().get(CONTENT_TYPE) and produces ct) to convert the final MIME token to a normalized form (lowercase) and preserve trimming so is_image() and image_format_code() will match correctly.
🧹 Nitpick comments (1)
app/src/components/channels/YuanbaoConfig.tsx (1)
50-64: ⚡ Quick winYuanbaoConfig: reconsider dispatching reset-from-mount for persisted “connecting”
- The repo’s ESLint config sets
react-hooks/set-state-in-effecttowarnwith “Allow initialization in effects”, so this may be treated as acceptable rehydration rather than a hard lint violation.- Still, this mount effect (lines 50-64) can produce a brief stale “connecting” UI before the correction; consider normalizing the persisted connection status earlier and deriving the effective status in render instead of dispatching on mount.
🤖 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/YuanbaoConfig.tsx` around lines 50 - 64, The useEffect in YuanbaoConfig currently dispatches setChannelConnectionStatus on mount to clear a persisted "connecting" state (it references channelConnections.connections.yuanbao and spec.mode); instead, remove that mount-time dispatch and instead derive an effective status at render time (e.g., compute effectiveStatus from channelConnections.connections.yuanbao?.[spec.mode] and fall back to 'disconnected' when it equals 'connecting' or is stale) so the UI never briefly shows stale "connecting"; alternatively, perform this normalization during rehydration/initialization in the store reducer where channelConnections is restored rather than in the component's useEffect.
🤖 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 `@src/openhuman/channels/controllers/ops.rs`:
- Around line 402-416: The preflight verifier is using the base Yuanbao config
instead of the user-supplied overrides parsed by opt_string (env_override,
api_domain_override, ws_domain_override, route_env_override), so
verify_yuanbao_credentials may test the wrong endpoint; fix by constructing the
effective Yuanbao config right after those opt_string calls (apply non-empty
overrides onto the original credentials/config), pass that effective config into
verify_yuanbao_credentials, and then persist the same effective config when
saving credentials so the runtime uses the identical endpoint on restart.
In `@src/openhuman/channels/providers/yuanbao/cos.rs`:
- Around line 476-484: The test
get_cos_credentials_sends_route_env_header_when_non_empty should also assert the
request path equals UPLOAD_INFO_PATH so it fails if the POST goes to the wrong
endpoint; update the wiremock expectation in that test to include a path matcher
for UPLOAD_INFO_PATH (use the UPLOAD_INFO_PATH symbol from the module) alongside
the existing header matcher so the mock only matches POSTs to the correct
endpoint with X-Route-Env: canary.
In `@src/openhuman/channels/runtime/startup.rs`:
- Around line 650-654: The code hydrates yb_cfg.app_secret from any stored
profile returned by auth.get_profile without verifying it matches the currently
configured yb_cfg.app_key; change the logic to only set yb_cfg.app_secret when
the stored profile's app_key equals the configured yb_cfg.app_key (e.g., check
profile.metadata.get("app_key") == Some(&yb_cfg.app_key) before assigning), or
alternatively query the encrypted store with a key that includes yb_cfg.app_key
so you cannot load a secret for a different app_key; update the branch around
auth.get_profile, the profile.metadata access, and the yb_cfg.app_secret
assignment to enforce this check.
---
Outside diff comments:
In `@src/openhuman/channels/providers/yuanbao/media.rs`:
- Around line 206-215: The extracted Content-Type string stored in ct should be
normalized (e.g., to lowercase and trimmed) before being returned so downstream
checks like is_image() and image_format_code()—which perform case-sensitive
comparisons—see a canonical MIME type; update the ct construction (the block
that reads headers().get(CONTENT_TYPE) and produces ct) to convert the final
MIME token to a normalized form (lowercase) and preserve trimming so is_image()
and image_format_code() will match correctly.
---
Nitpick comments:
In `@app/src/components/channels/YuanbaoConfig.tsx`:
- Around line 50-64: The useEffect in YuanbaoConfig currently dispatches
setChannelConnectionStatus on mount to clear a persisted "connecting" state (it
references channelConnections.connections.yuanbao and spec.mode); instead,
remove that mount-time dispatch and instead derive an effective status at render
time (e.g., compute effectiveStatus from
channelConnections.connections.yuanbao?.[spec.mode] and fall back to
'disconnected' when it equals 'connecting' or is stale) so the UI never briefly
shows stale "connecting"; alternatively, perform this normalization during
rehydration/initialization in the store reducer where channelConnections is
restored rather than in the component's useEffect.
🪄 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: 38946162-7261-49db-b5f2-7303b7e0457f
📒 Files selected for processing (30)
app/src/components/channels/ChannelSetupModal.tsxapp/src/components/channels/YuanbaoConfig.tsxapp/src/lib/i18n/chunks/ar-3.tsapp/src/lib/i18n/chunks/bn-3.tsapp/src/lib/i18n/chunks/de-3.tsapp/src/lib/i18n/chunks/en-3.tsapp/src/lib/i18n/chunks/es-3.tsapp/src/lib/i18n/chunks/fr-3.tsapp/src/lib/i18n/chunks/hi-3.tsapp/src/lib/i18n/chunks/id-3.tsapp/src/lib/i18n/chunks/it-3.tsapp/src/lib/i18n/chunks/ko-3.tsapp/src/lib/i18n/chunks/pt-3.tsapp/src/lib/i18n/chunks/ru-3.tsapp/src/lib/i18n/chunks/zh-CN-3.tsapp/src/lib/i18n/en.tsapp/src/lib/i18n/ko.tssrc/openhuman/channels/controllers/ops.rssrc/openhuman/channels/controllers/ops_tests.rssrc/openhuman/channels/providers/yuanbao/channel.rssrc/openhuman/channels/providers/yuanbao/connection.rssrc/openhuman/channels/providers/yuanbao/cos.rssrc/openhuman/channels/providers/yuanbao/media.rssrc/openhuman/channels/providers/yuanbao/outbound.rssrc/openhuman/channels/providers/yuanbao/proto.rssrc/openhuman/channels/providers/yuanbao/proto_biz.rssrc/openhuman/channels/providers/yuanbao/sign.rssrc/openhuman/channels/providers/yuanbao/types.rssrc/openhuman/channels/providers/yuanbao/wire.rssrc/openhuman/channels/runtime/startup.rs
✅ Files skipped from review due to trivial changes (7)
- app/src/lib/i18n/chunks/en-3.ts
- app/src/lib/i18n/chunks/pt-3.ts
- app/src/lib/i18n/chunks/ko-3.ts
- app/src/lib/i18n/chunks/fr-3.ts
- app/src/lib/i18n/chunks/it-3.ts
- app/src/lib/i18n/en.ts
- app/src/lib/i18n/chunks/ru-3.ts
lrt4836
added a commit
to YuanbaoTeam/openhuman
that referenced
this pull request
May 23, 2026
…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.
lrt4836
force-pushed
the
feat/yuanbao-channel
branch
from
36a290e to
b2f3d21
Compare
lrt4836
added a commit
to YuanbaoTeam/openhuman
that referenced
this pull request
May 24, 2026
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.
lrt4836
added a commit
to YuanbaoTeam/openhuman
that referenced
this pull request
May 24, 2026
…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.
lrt4836
added a commit
to YuanbaoTeam/openhuman
that referenced
this pull request
May 24, 2026
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%.
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.
lrt4836
force-pushed
the
feat/yuanbao-channel
branch
from
426263a to
2fd5746
Compare
Member
Post-merge-with-main fixes (from
|
7 tasks
Member
Redoing this PR in #2600This PR did not have "Allow edits from maintainers" enabled, so I could not push the necessary fixes (merge conflict resolution + code improvements) directly to The work has been carried forward in #2600, which includes:
Thank you @lrt4836 for the contribution! The channel implementation is solid and all original CodeRabbit findings were properly addressed. You'll get the attribution. well done! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Yuanbaochannel provider (Tencent 元宝 robot logic v5) undersrc/openhuman/channels/providers/yuanbao/, wired into the channel registry + startup loop and exposed to the UI through the existingChannelSetupModalflow.sign.rs) with cached tokens, message splitting (splitter.rs), and a COS uploader (cos.rs) for media.connection.rs+wire.rs+proto_biz.rs) with bounded reconnect, single-flight token refresh oncode=10099, prompt shutdown response, and bounded length-delimited parsing.YuanbaoConfig.tsxform (i18n-keyed validation messages),YuanbaoIcon, andChannelSetupModalintegration for icon parity withChannelSelector.app_secretis verified at connect time via the live sign-token endpoint, persisted only in the encrypted credentials store (never in plaintextconfig.toml), and hydrated at startup with a stale-app_keyguard.YuanbaoIcon,YuanbaoConfig,ChannelSetupModal,channelConnectionsSlice) — diff-cover on changed lines: 13% → 84%, well above the ≥80% gate.Problem
Yuanbao was not a supported channel; users on Tencent Yuanbao bots had no first-class way to wire OpenHuman to their robot. The 1st-round CodeRabbit review additionally flagged:
ChannelSetupModalheader.connect_channelreturningconnectedeven when the upstream sign-token call failed.app_secretmirrored in plaintextconfig.toml.as i32/as u32varint conversions in protobuf decoding.wire::parse_fields.2nd-round CodeRabbit then flagged three follow-ups on the above fixes:
env/api_domain/route_envoverrides — apreconnect could verify against PROD then fail after restart.UPLOAD_INFO_PATH.resolve_yuanbao_app_secretcopied the stored secret even whenyb_cfg.app_keyhad been changed — pairing a fresh key with a stale secret on next startup.Solution
controllers/ops.rs: extractedbuild_effective_yuanbao_configthat overlays client overrides on top of base TOML and callsapply_env_defaults. The verifier consumes the resultingYuanbaoConfig, and the same config is reused for persistence — verify and persist can no longer diverge.providers/yuanbao/cos.rs: bound the route-env wiremock matcher toUPLOAD_INFO_PATHas well.runtime/startup.rs:resolve_yuanbao_app_secretnow comparesprofile.metadata.get("app_key")againstyb_cfg.app_keybefore copying; on mismatch it logs both keys and leavesapp_secretempty soYuanbaoChannel::new'svalidate()step fails loudly.providers/yuanbao/proto_biz.rs: introducedvarint_to_i32/varint_to_u32helpers that returnYuanbaoError::ProtoDecodeon overflow.providers/yuanbao/wire.rs:usize::try_from(len)+pos.checked_add(len_usize)in theWT_LENbranch.providers/yuanbao/connection.rs: added an early*shutdown.borrow()check afterconnect_once()so the loop exits without a backoff delay.providers/yuanbao/channel.rs: drop the hex preview ofbiz_bodyonPipelineOutcome::Failed; log onlybiz_body.len().channels.yuanbao.*keys (connecting,fieldRequired,unexpectedStatus) added toen.ts+zh-CN-3.ts(translated) and the other 12 locale chunks (English placeholders).de-3.ts/de-5.tsthat were trippingTS1117inBuild E2E app.Submission Checklist
diff-cover) meet the gate enforced by.github/workflows/coverage.yml. Runpnpm test:coverageandpnpm test:rustlocally; PRs below 80% on changed lines will not merge.docs/TEST-COVERAGE-MATRIX.mdreflect this change (orN/A: behaviour-only change)## Relateddocs/RELEASE-MANUAL-SMOKE.md)Closes #NNNin the## RelatedsectionImpact
app_secretnever lands in plaintextconfig.toml; runtime loads it from the encrypted credentials store at startup. Stale-app_keyprofiles will no longer be paired with the new key.app_secretwritten by a prior (pre-merge) version of this branch intoconfig.tomlis intentionally not auto-migrated; on first launch the runtime warnsyuanbao credentials missing — connect the channel again from the UI. Users reconnect once to populate the encrypted store.ProtoDecodeerrors instead of silently truncating overflowing varints or panicking on oversized length-delimited fields.Related
10.1.5 Yuanbao Connection(added; layerRU, status 🟡).10.1.5from 🟡 to ✅).channels.yuanbao.*strings in the 12 non-en/ non-zh-CNlocales (placeholders only today).AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
YuanbaoTeam:feat/yuanbao-channel426263a9(HEAD at time of writing)Validation Run
pnpm --filter openhuman-app format:checkpnpm typecheckcargo test --lib -- yuanbao→ 190 passed, 0 failed; VitestYuanbaoIcon3/3,YuanbaoConfig13/13,ChannelSetupModal4/4,channelConnectionsSlice10/10 — all passing. Localdiff-coveron changed lines = 84%.cargo fmt --check→ exit 0app/src-tauri/**changes in this PR. Tauri fmt/check (if changed):Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
app_key+app_secret(+ optionalenv/api_domain/ws_domain/route_envoverrides) and the channel goes live after a restart.Parity Contract
connect_channel/disconnect_channel/channel_status.def.validate_credentialsruns first as for every other channel;verify_yuanbao_credentialsis yuanbao-specific and short-circuits on upstream API error before any persistence; startup hydration is best-effort (warn-only) and never panics.Duplicate / Superseded PR Handling
tinyhumansai/openhuman#2494).