feat(channels/yuanbao): add Yuanbao channel provider#2494
Open
lrt4836 wants to merge 1 commit into
Open
Conversation
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.
Contributor
📝 WalkthroughWalkthroughThis PR adds a complete new Yuanbao instant-messaging channel integration to OpenHuman. Frontend adds React components for authentication UI and icon rendering. Backend implements a full WebSocket provider with protocol codecs, inbound message filtering pipeline, outbound sending, token signing, and Tencent COS media upload support. ChangesYuanbao Channel Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
|
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
Comment on lines
+68
to
69
| const emojiIcon = CHANNEL_ICONS[definition.icon] ?? ''; | ||
|
|
Contributor
There was a problem hiding this comment.
Yuanbao modal header icon currently resolves to empty.
emojiIcon now comes from CHANNEL_ICONS[definition.icon], but this map has no yuanbao entry in this file, so the new channel header shows no icon. Add a Yuanbao mapping (or render YuanbaoIcon here as a special case).
Also applies to: 92-92
🤖 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/ChannelSetupModal.tsx` around lines 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).
| for (const field of spec.fields) { | ||
| const empty = !fieldValues[field.key]?.trim(); | ||
| if (field.required && empty) { | ||
| errors[field.key] = `${field.label} 不能为空`; |
Contributor
There was a problem hiding this comment.
Validation copy is hardcoded and bypasses i18n.
'不能为空' should come from translated resources to keep locale behavior consistent with the rest of the modal.
🤖 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` at 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.
Comment on lines
+112
to
+117
| const result = await channelConnectionsApi.connectChannel('yuanbao', { | ||
| authMode: spec.mode, | ||
| credentials, | ||
| }); | ||
| console.log('[YuanbaoConfig] handleConnect: 5.RPC returned', result); | ||
| log('connect result: %o', result); |
Contributor
There was a problem hiding this comment.
Don’t force connected without validating connectChannel result status.
handleConnect marks Yuanbao as connected even if the API returns a non-connected status (e.g., pending flow). Branch on result.status first and only dispatch connected on explicit success.
Suggested fix
const result = await channelConnectionsApi.connectChannel('yuanbao', {
authMode: spec.mode,
credentials,
});
- console.log('[YuanbaoConfig] handleConnect: 5.RPC returned', result);
- log('connect result: %o', result);
+ log('connect result: %o', { status: result.status, restart_required: result.restart_required });
+
+ if (result.status !== 'connected') {
+ dispatch(
+ setChannelConnectionStatus({
+ channel: 'yuanbao',
+ authMode: spec.mode,
+ status: 'disconnected',
+ lastError: result.message ?? `Unexpected connect status: ${result.status}`,
+ })
+ );
+ return;
+ }Also applies to: 119-164
🤖 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 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.
Comment on lines
+399
to
+407
| let mut persisted = config.clone(); | ||
| let mut yb_config = persisted | ||
| .channels_config | ||
| .yuanbao | ||
| .clone() | ||
| .unwrap_or_default(); | ||
| yb_config.app_key = app_key; | ||
| yb_config.app_secret = app_secret; | ||
| persisted.channels_config.yuanbao = Some(yb_config); |
Contributor
There was a problem hiding this comment.
Persist Yuanbao endpoint selection (env / api_domain) during connect.
The Yuanbao branch only writes app_key/app_secret. If the client submits non-default environment selection, it is dropped and runtime can reconnect to the wrong endpoint after restart.
🔧 Suggested fix
let mut yb_config = persisted
.channels_config
.yuanbao
.clone()
.unwrap_or_default();
+ if let Some(env) = creds_map
+ .get("env")
+ .and_then(|v| v.as_str())
+ .map(str::trim)
+ .filter(|s| !s.is_empty())
+ {
+ yb_config.route_env = env.to_string();
+ }
+ if let Some(api_domain) = creds_map
+ .get("api_domain")
+ .and_then(|v| v.as_str())
+ .map(str::trim)
+ .filter(|s| !s.is_empty())
+ {
+ yb_config.api_domain = api_domain.to_string();
+ } else if yb_config.api_domain.is_empty() {
+ yb_config.apply_env_defaults();
+ }
yb_config.app_key = app_key;
yb_config.app_secret = app_secret;
persisted.channels_config.yuanbao = Some(yb_config);🤖 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 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).
Comment on lines
+405
to
+407
| yb_config.app_key = app_key; | ||
| yb_config.app_secret = app_secret; | ||
| persisted.channels_config.yuanbao = Some(yb_config); |
Contributor
There was a problem hiding this comment.
Do not persist app_secret in plaintext in config.toml.
Line 406 writes the raw secret into persisted channel config. That expands exposure surface compared with credential-store-only handling.
🤖 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 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.
Comment on lines
+408
to
+417
| let preview_len = biz_body.len().min(256); | ||
| let hex: String = biz_body[..preview_len] | ||
| .iter() | ||
| .map(|b| format!("{b:02x}")) | ||
| .collect(); | ||
| warn!( | ||
| "[yuanbao] pipeline error: {err} | biz_len={} biz_hex_first_{preview_len}={}", | ||
| biz_body.len(), | ||
| hex | ||
| ); |
Contributor
There was a problem hiding this comment.
Avoid logging raw inbound payload bytes on pipeline errors.
Line 408–417 logs a hex preview of the inbound business payload. This can leak user content/PII into logs.
🔧 Suggested fix
- let preview_len = biz_body.len().min(256);
- let hex: String = biz_body[..preview_len]
- .iter()
- .map(|b| format!("{b:02x}"))
- .collect();
warn!(
- "[yuanbao] pipeline error: {err} | biz_len={} biz_hex_first_{preview_len}={}",
- biz_body.len(),
- hex
+ "[yuanbao] pipeline error: {err} | biz_len={} cmd={} module={}",
+ biz_body.len(),
+ frame.cmd,
+ frame.module
);As per coding guidelines: “Never log secrets, raw JWTs, API keys, or full PII — redact or omit sensitive fields.”
🤖 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 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.
Comment on lines
+217
to
+234
| self.set_state(ConnectionState::Disconnected); | ||
| *self.sender.lock().await = None; | ||
| self.pending.lock().clear(); | ||
|
|
||
| attempt += 1; | ||
| let delay = backoff_seconds(attempt); | ||
| info!( | ||
| "[yuanbao] reconnecting in {}s (attempt {}/{})", | ||
| delay, attempt, max_attempts | ||
| ); | ||
| tokio::select! { | ||
| _ = time::sleep(Duration::from_secs(delay)) => {} | ||
| _ = shutdown.changed() => { | ||
| info!("[yuanbao] shutdown received during backoff"); | ||
| self.shutdown().await; | ||
| return; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid delayed shutdown after connect_once exits on shutdown signal.
When connect_once returns because shutdown.changed() fired, run() still executes backoff sleep before the next loop check. This can delay stop by up to the current reconnect delay. Please short-circuit immediately after cleanup when shutdown is already true (Line 227 onward path).
🔧 Suggested fix
self.set_state(ConnectionState::Disconnected);
*self.sender.lock().await = None;
self.pending.lock().clear();
+ if *shutdown.borrow() {
+ info!("[yuanbao] shutdown observed after disconnect cleanup");
+ self.shutdown().await;
+ return;
+ }
+
attempt += 1;
let delay = backoff_seconds(attempt);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| self.set_state(ConnectionState::Disconnected); | |
| *self.sender.lock().await = None; | |
| self.pending.lock().clear(); | |
| attempt += 1; | |
| let delay = backoff_seconds(attempt); | |
| info!( | |
| "[yuanbao] reconnecting in {}s (attempt {}/{})", | |
| delay, attempt, max_attempts | |
| ); | |
| tokio::select! { | |
| _ = time::sleep(Duration::from_secs(delay)) => {} | |
| _ = shutdown.changed() => { | |
| info!("[yuanbao] shutdown received during backoff"); | |
| self.shutdown().await; | |
| return; | |
| } | |
| } | |
| self.set_state(ConnectionState::Disconnected); | |
| *self.sender.lock().await = None; | |
| self.pending.lock().clear(); | |
| if *shutdown.borrow() { | |
| info!("[yuanbao] shutdown observed after disconnect cleanup"); | |
| self.shutdown().await; | |
| return; | |
| } | |
| attempt += 1; | |
| let delay = backoff_seconds(attempt); | |
| info!( | |
| "[yuanbao] reconnecting in {}s (attempt {}/{})", | |
| delay, attempt, max_attempts | |
| ); | |
| tokio::select! { | |
| _ = time::sleep(Duration::from_secs(delay)) => {} | |
| _ = shutdown.changed() => { | |
| info!("[yuanbao] shutdown received during backoff"); | |
| self.shutdown().await; | |
| return; | |
| } | |
| } |
🤖 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 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.
Comment on lines
+247
to
+248
| code: get_varint(&fields, 1) as i32, | ||
| message: get_string(&fields, 2), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all narrowing casts applied directly to decoded protobuf varints in this module.
rg -nP 'get_varint\([^)]*\)\s+as\s+(i32|u32)' src/openhuman/channels/providers/yuanbao/proto_biz.rsRepository: tinyhumansai/openhuman
Length of output: 1966
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/openhuman/channels/providers/yuanbao/proto_biz.rs"
# 1) List all occurrences of narrowing casts from get_varint(...)
rg -nP 'get_varint\([^)]*\)\s+as\s+(i32|u32)\b' "$FILE" || true
# 2) Print the most relevant line ranges around the previously mentioned locations
sed -n '220,340p' "$FILE" | nl -ba | sed -n '1,140p'
sed -n '340,420p' "$FILE" | nl -ba | sed -n '1,120p'Repository: tinyhumansai/openhuman
Length of output: 510
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/openhuman/channels/providers/yuanbao/proto_biz.rs"
# Print context around the reported cast lines (using awk for line numbers)
for start in 235 250 285 292 296 305 310; do
end=$((start+25))
echo "---- ${start}-${end} ----"
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%d:%s\n", NR, $0}' "$FILE"
doneRepository: tinyhumansai/openhuman
Length of output: 6845
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden --no-ignore-vcs "fn\s+get_varint|get_varint<|get_varint\s*\(" src/openhuman/channels/providers/yuanbao/proto_biz.rs
rg -n --hidden --no-ignore-vcs "get_varint" src/openhuman/channels/providers/yuanbao/proto_biz.rsRepository: tinyhumansai/openhuman
Length of output: 1041
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the actual definition(s) of get_varint in the Rust codebase
rg -n --hidden --no-ignore-vcs "fn\s+get_varint" src/openhuman
rg -n --hidden --no-ignore-vcs "get_varint\s*<|pub\s+fn\s+get_varint|pub\s+.*get_varint" src/openhumanRepository: tinyhumansai/openhuman
Length of output: 306
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/openhuman/channels/providers/yuanbao/wire.rs"
for start in 110 130 145 155 170; do
end=$((start+40))
echo "---- ${start}-${end} ----"
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%d:%s\n", NR, $0}' "$FILE"
doneRepository: tinyhumansai/openhuman
Length of output: 6283
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/openhuman/channels/providers/yuanbao/wire.rs"
# Show FieldValue + varint parsing logic
for start in 1 40 70 90; do
end=$((start+55))
echo "---- ${start}-${end} ----"
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%d:%s\n", NR, $0}' "$FILE"
done
# Also show where FieldValue::Varint is produced
rg -n "FieldValue::Varint" "$FILE"Repository: tinyhumansai/openhuman
Length of output: 8184
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "enum YuanbaoError|ProtoDecode" src/openhuman/channels/providers/yuanbaoRepository: tinyhumansai/openhuman
Length of output: 1278
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/openhuman/channels/providers/yuanbao/proto_biz.rs"
# Show types that receive the casts
rg -n "struct GroupInfo|struct GroupMemberListPage|struct GroupMember" "$FILE"
for m in $(rg -n "struct GroupInfo|struct GroupMemberListPage|struct GroupMember" "$FILE" | cut -d: -f1); do
start=$((m-15)); end=$((m+120));
echo "---- ${start}-${end} ----"
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%d:%s\n", NR, $0}' "$FILE"
doneRepository: tinyhumansai/openhuman
Length of output: 48
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/openhuman/channels/providers/yuanbao/proto_biz.rs"
# Get line numbers for struct declarations
rg -n "struct\s+(GroupInfo|GroupMemberListPage|GroupMember)\b" "$FILE" | while IFS=: read -r ln rest; do
start=$((ln-20)); end=$((ln+80))
echo "---- struct context around line $ln ($start-$end) ----"
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%d:%s\n", NR, $0}' "$FILE"
doneRepository: tinyhumansai/openhuman
Length of output: 48
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/openhuman/channels/providers/yuanbao/proto_biz.rs"
rg -n "GroupInfo|GroupMemberListPage|GroupMember\b" "$FILE" | head -n 80
# Print around the first few hits to see whether these are struct definitions or imported types
# (use fixed line windows around likely definition sites)
for ln in $(rg -n "GroupInfo|GroupMemberListPage|GroupMember\b" "$FILE" | cut -d: -f1 | head -n 8); do
start=$((ln-20)); end=$((ln+60))
echo "---- context around ${ln} (${start}-${end}) ----"
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%d:%s\n", NR, $0}' "$FILE"
doneRepository: tinyhumansai/openhuman
Length of output: 17514
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "struct\s+(GroupInfo|GroupMemberListPage|GroupMember)\b" src/openhuman/channels/providers/yuanbaoRepository: tinyhumansai/openhuman
Length of output: 309
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/openhuman/channels/providers/yuanbao/types.rs"
# Print around the struct definitions
for ln in 210 220 226 232 236 238 244; do
start=$((ln-15)); end=$((ln+60))
echo "---- window ${start}-${end} ----"
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%d:%s\n", NR, $0}' "$FILE" | head -n 120
doneRepository: tinyhumansai/openhuman
Length of output: 11604
Guard varint-to-int conversions against overflow in Yuanbao decoders
get_varint(...) returns a u64, but the decoder narrows with as i32 / as u32 for several response fields; out-of-range (malformed) values will truncate/wrap instead of failing, corrupting code, member_count, pagination (next_offset), and member metadata (role, join_time).
Cast sites: 247, 257, 294, 295, 300, 303, 315.
Use i32::try_from(...) / u32::try_from(...) and return YuanbaoError::ProtoDecode(...) when conversion fails.
🤖 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/proto_biz.rs` around lines 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.
Comment on lines
+101
to
+111
| let (len, n) = decode_varint(data, pos)?; | ||
| pos += n; | ||
| let end = pos + len as usize; | ||
| if end > data.len() { | ||
| return Err(YuanbaoError::ProtoDecode(format!( | ||
| "truncated len field {field}: need {len} have {}", | ||
| data.len() - pos | ||
| ))); | ||
| } | ||
| out.push((field, FieldValue::Bytes(data[pos..end].to_vec()))); | ||
| pos = end; |
Contributor
There was a problem hiding this comment.
Guard length-delimited field arithmetic with checked conversions.
Line 103 does unchecked pos + len as usize. A crafted large varint length can overflow and panic during slicing.
🔧 Suggested fix
- let end = pos + len as usize;
+ let len_usize = usize::try_from(len).map_err(|_| {
+ YuanbaoError::ProtoDecode(format!("len too large for usize at field {field}"))
+ })?;
+ let end = pos.checked_add(len_usize).ok_or_else(|| {
+ YuanbaoError::ProtoDecode(format!("len overflow at field {field}"))
+ })?;
if end > data.len() {
return Err(YuanbaoError::ProtoDecode(format!(
"truncated len field {field}: need {len} have {}",
data.len() - pos
)));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let (len, n) = decode_varint(data, pos)?; | |
| pos += n; | |
| let end = pos + len as usize; | |
| if end > data.len() { | |
| return Err(YuanbaoError::ProtoDecode(format!( | |
| "truncated len field {field}: need {len} have {}", | |
| data.len() - pos | |
| ))); | |
| } | |
| out.push((field, FieldValue::Bytes(data[pos..end].to_vec()))); | |
| pos = end; | |
| let (len, n) = decode_varint(data, pos)?; | |
| pos += n; | |
| let len_usize = usize::try_from(len).map_err(|_| { | |
| YuanbaoError::ProtoDecode(format!("len too large for usize at field {field}")) | |
| })?; | |
| let end = pos.checked_add(len_usize).ok_or_else(|| { | |
| YuanbaoError::ProtoDecode(format!("len overflow at field {field}")) | |
| })?; | |
| if end > data.len() { | |
| return Err(YuanbaoError::ProtoDecode(format!( | |
| "truncated len field {field}: need {len} have {}", | |
| data.len() - pos | |
| ))); | |
| } | |
| out.push((field, FieldValue::Bytes(data[pos..end].to_vec()))); | |
| pos = end; |
🤖 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/wire.rs` around lines 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.
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
1 participant
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.
Adds a new Yuanbao channel provider so OpenHuman can talk to the Yuanbao bot service over signed WebSocket + HTTPS.
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.YuanbaoConfigform + icon, hooked into the channel selector, setup modal, and connection slice using the existing per-channel definition pattern.envfield; no test credentials in code.Summary
Problem
Solution
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
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
Validation Run
pnpm --filter openhuman-app format:checkpnpm typecheckValidation Blocked
command:error:impact:Behavior Changes
Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit