Repository navigation
refactor: split god-files into focused sibling modules (#835) - #1010
Merged
Merged
Conversation
) Decomposes 5 oversized source files (telegram/channel.rs, composio/catalogs.rs, memory/ingestion.rs, providers/compatible.rs, skills/ops.rs) into focused sibling modules by responsibility: _core, _ops, _types, _parse, _send/_recv, _rules. Also splits browser tool helpers into action_parser, native_backend, security, and types modules. Re-exports from each mod.rs preserve the public interface unchanged. Fixes forward-compat with upstream additions (Skill.dir_name, Skill.read_body, uninstall_skill, TelegramChannel.silent_streaming) that landed on main after the branch cut.
Contributor
📝 WalkthroughWalkthroughLarge-scale refactoring that decomposes six monolithic modules (telegram channel, composio catalogs, memory ingestion, OpenAI provider, skills operations, browser tools) into focused submodules, re-exporting public APIs from parent modules to maintain backward compatibility while improving code organization. Changes
Sequence Diagram(s)Not applicable—changes are primarily refactoring and modularization without introducing new features or significantly altering control flow. Existing interactions between components remain unchanged. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Review requires verifying correct module boundaries and re-export wiring across six separate decompositions, ensuring no functionality was inadvertently lost in reorganization, confirming API surface consistency, and spot-checking new implementations (particularly NativeBrowserState WebDriver integration). Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![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: 17
🧹 Nitpick comments (5)
src/openhuman/tools/impl/browser/browser.rs (1)
8-16: Unusual#[path = "..."]attribute usage for same-directory modules.When module files are in the same directory as the declaring file, Rust's default module resolution should work without explicit
#[path]attributes. Usingmod action_parser;instead of#[path = "action_parser.rs"] mod action_parser;is idiomatic and reduces boilerplate.♻️ Suggested simplification
-#[path = "action_parser.rs"] -mod action_parser; +mod action_parser; #[cfg(feature = "browser-native")] -#[path = "native_backend.rs"] mod native_backend; -#[path = "security.rs"] mod security; -#[path = "types.rs"] mod types;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/tools/impl/browser/browser.rs` around lines 8 - 16, The module declarations use explicit #[path = "..."] attributes unnecessarily; replace lines like #[path = "action_parser.rs"] mod action_parser; (and similarly for native_backend, security, types) with the idiomatic plain module declarations mod action_parser;, mod security;, mod types; and keep the conditional attribute only for native_backend as #[cfg(feature = "browser-native")] mod native_backend; so Rust's default same-directory resolution is used and the cfg is preserved.src/openhuman/tools/impl/browser/security.rs (2)
36-61: Host extraction handles common cases but consider usingurlcrate.The manual parsing handles
http://,https://,file://, and IPv6 brackets. However, edge cases like userinfo (http://user:pass@host/) or unusual schemes may not be handled correctly. Sincereqwest::Urlis already a dependency (used inendpoint_reachable), consider using it here for robustness.♻️ Optional: Use reqwest::Url for robust parsing
pub(crate) fn extract_host(url_str: &str) -> anyhow::Result<String> { let parsed = reqwest::Url::parse(url_str.trim()) .map_err(|_| anyhow::anyhow!("Invalid URL: {url_str}"))?; parsed .host_str() .filter(|h| !h.is_empty()) .map(|h| h.to_lowercase()) .ok_or_else(|| anyhow::anyhow!("Invalid URL: no host")) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/tools/impl/browser/security.rs` around lines 36 - 61, The current extract_host function manually parses URL strings and can miss cases like userinfo or unusual schemes; replace its manual parsing with robust parsing using reqwest::Url (already used in endpoint_reachable). Update extract_host to parse url_str.trim() via reqwest::Url::parse, return an error mapped to anyhow::Error on parse failure, then call host_str(), ensure it is non-empty, lowercase it and return it, otherwise bail with "Invalid URL: no host". Keep the function signature extract_host(url_str: &str) -> anyhow::Result<String>.
129-134: Case-insensitive env var matching could be simplified.The truthy value matching handles several cases but misses common variants like
"True"or"Yes". Consider usingeq_ignore_ascii_casefor more robust matching.♻️ More robust truthy check
pub(crate) fn allow_all_browser_domains() -> bool { - matches!( - std::env::var("OPENHUMAN_BROWSER_ALLOW_ALL").ok().as_deref(), - Some("1") | Some("true") | Some("TRUE") | Some("yes") | Some("YES") - ) + std::env::var("OPENHUMAN_BROWSER_ALLOW_ALL") + .ok() + .map(|v| matches!(v.trim(), "1") || v.eq_ignore_ascii_case("true") || v.eq_ignore_ascii_case("yes")) + .unwrap_or(false) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/tools/impl/browser/security.rs` around lines 129 - 134, The current allow_all_browser_domains function manually matches several case variants and misses values like "True" or "Yes"; update it to normalize case using eq_ignore_ascii_case (or to_lowercase) and check canonical truthy values. Locate allow_all_browser_domains and replace the matches! block with logic that obtains the env var (std::env::var("OPENHUMAN_BROWSER_ALLOW_ALL").ok().as_deref()) and returns true when the string is "1" or equals_ignore_ascii_case "true" or "yes" (e.g., s == "1" || s.eq_ignore_ascii_case("true") || s.eq_ignore_ascii_case("yes")). Ensure the function still returns a bool and preserves the ok().as_deref() flow.src/openhuman/providers/compatible_dump.rs (1)
50-59: Consider extracting the duplicated model name sanitization.The character sanitization logic for
safe_modelis identical in bothdump_prompt_if_enabledanddump_response_if_enabled. While acceptable for a debug utility, a small helper function would reduce duplication.♻️ Optional refactor to extract sanitization
+fn sanitize_model_name(model: &str) -> String { + model + .chars() + .map(|c| { + if c.is_alphanumeric() || c == '-' || c == '_' || c == '.' { + c + } else { + '_' + } + }) + .collect() +} + pub(crate) fn dump_prompt_if_enabled<T: Serialize>( // ... - let safe_model: String = model - .chars() - .map(|c| { - if c.is_alphanumeric() || c == '-' || c == '_' || c == '.' { - c - } else { - '_' - } - }) - .collect(); + let safe_model = sanitize_model_name(model);Also applies to: 98-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/providers/compatible_dump.rs` around lines 50 - 59, The model-name sanitization used to build safe_model is duplicated in dump_prompt_if_enabled and dump_response_if_enabled; extract that logic into a small helper function (e.g., fn sanitize_model_name(model: &str) -> String) that applies the same char filtering (allow alphanumeric, '-', '_', '.') and returns the sanitized String, then replace the inline map/collect blocks in both dump_prompt_if_enabled and dump_response_if_enabled to call sanitize_model_name(model); ensure the helper is exported/visible to both functions and update any tests or callers if necessary.src/openhuman/skills/ops_create.rs (1)
265-305:yaml_scalarhandles common cases but has edge case gaps.The function correctly quotes strings with YAML-significant characters and escapes control characters. However, it doesn't handle values that YAML would interpret as special types (e.g.,
"true","false","null","123","1.5","~"). For skill metadata (names, descriptions, author names), this is likely acceptable, but if tags ever contain these values, they'll be interpreted differently when re-parsed.💡 Optional: extend needs_quote check for type-ambiguous values
let needs_quote = s.is_empty() || s.chars().any(|c| { matches!( c, ':' | '#' | '\'' | '"' | '\n' | '\r' | '\t' | '[' | ']' | '{' | '}' | ',' | '&' | '*' | '!' | '|' | '>' | '%' | '@' | '`' ) }) || s.starts_with(|c: char| c.is_ascii_whitespace() || c == '-' || c == '?') - || s.ends_with(|c: char| c.is_ascii_whitespace()); + || s.ends_with(|c: char| c.is_ascii_whitespace()) + || matches!(s.to_ascii_lowercase().as_str(), "true" | "false" | "null" | "~" | "yes" | "no" | "on" | "off") + || s.parse::<f64>().is_ok();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/skills/ops_create.rs` around lines 265 - 305, yaml_scalar currently decides to skip quoting based on characters only, which lets strings like "true", "false", "null", "~", integer/float forms (e.g., "123", "1.5") be emitted unquoted and therefore parsed as non-strings by YAML consumers; update yaml_scalar to detect and force-quote these type-ambiguous values by adding an additional needs_quote condition that returns true when s matches YAML booleans ("true"/"false", case-insensitive as needed), explicit null tokens ("null", "Null", "~"), or numeric patterns (integers, floats, scientific notation), or the tilde "~"; implement these checks using simple string comparisons and/or a compact regex and keep the existing escaping/quoting behavior in yaml_scalar.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/openhuman/channels/providers/telegram/channel_core.rs`:
- Around line 28-33: The constructor new() creates a plain reqwest::Client while
http_client() builds a proxy-aware client on each call, causing inconsistent
proxy handling and no shared connection pool; change new() to construct a single
proxy-configured reqwest::Client (using the same logic currently in
http_client()) and store it in self.client, then make http_client() return a
reference to that stored client (or remove http_client() and update users to use
self.client) so all paths (draft/edit/delete and polling/send) use the same
pooled, proxy-aware client; update references to Self::new,
ChannelCore::http_client, and any uses of self.client to ensure the shared
client is used everywhere.
- Around line 17-23: The pairing code is being printed to stdout via println! in
the block that constructs a PairingGuard (see PairingGuard::new and pairing_code
usage) which exposes a bearer secret; instead, remove the println! calls and
expose the code via a secure interactive/UI pathway: have the function return
the PairingGuard (or its pairing_code) to the caller (or emit an event/callback)
so the higher-level UI can display it securely, and update any callers of this
block to handle and surface the code through the UI flow tied to
TELEGRAM_BIND_COMMAND rather than logging it to the console.
In `@src/openhuman/channels/providers/telegram/channel_recv.rs`:
- Around line 298-302: The warning currently emits full PII (username and
sender_id_str) in the tracing::warn call; replace those literal identifiers with
a redacted or hashable representation (e.g., a stable HMAC/sha256 fingerprint or
a correlation id) before logging and use that redacted value in the
tracing::warn message for the current block (the tracing::warn that references
username and sender_id_str). Apply the same redaction approach to the other new
Telegram sender/recipient logs introduced in this split so no raw Telegram
usernames or numeric IDs are emitted.
- Around line 8-16: The code in typing_body_for_recipient currently inserts
thread IDs as JSON strings, which Telegram requires to be numeric; update
typing_body_for_recipient to parse the thread_id returned by
Self::parse_reply_target into an integer type (e.g., i64 or u64) and set
body["message_thread_id"] to a serde_json::Value::Number created from that
integer (instead of a String). Ensure you handle parse errors (returning no
thread_id or logging) so you only insert a numeric serde_json::Number when
parsing succeeds.
In `@src/openhuman/channels/providers/telegram/channel_send.rs`:
- Around line 215-228: The success log in channel_send.rs currently logs the
full outbound URL (see the tracing::info!("Telegram {method} sent to {chat_id}:
{url}") call after the HTTP send), which may leak presigned/tokenized URLs;
change that tracing::info call (and the similar tracing::info calls referenced
later) to redact the full URL: derive and log only the media kind and a safe
identifier such as the URL host and filename/basename (or replace the URL with
"<redacted-url>") instead of the complete {url}; make this change where the code
builds requests via self.http_client(), self.api_url(method), and where
variables named url are interpolated into logs (also update the analogous
logging sites for other media-send branches noted in the review).
In `@src/openhuman/channels/providers/telegram/channel_types.rs`:
- Around line 37-44: The typing state is currently global (typing_handle:
Mutex<Option<TelegramTypingTask>>) so concurrent chats stomp each other; change
typing_handle on the TelegramChannel to be a per-recipient map (e.g.,
Mutex<HashMap<String, TelegramTypingTask>> keyed by chat/recipient id or
username), then update start_typing to insert/replace the task under the
recipient key and only cancel the previous task for that recipient, and update
stop_typing to look up and remove/cancel the task for the specific recipient
instead of ignoring the recipient parameter; update any other codepaths that
read typing_handle to use the keyed map (TelegramChannel, typing_handle,
TelegramTypingTask, start_typing, stop_typing).
In `@src/openhuman/memory/ingestion_parse.rs`:
- Around line 278-287: The current line-based extraction loop uses chunk_index
as the relation order so multiple extractions from the same chunk share the same
order_index; instead add a dedicated monotonic counter (e.g., order_counter:
usize or i64) initialized before the loop and increment it for every extracted
RawRelation; in the loop (where chunk_hint, find_chunk_index and chunk_index are
computed) set order_index by converting this counter (not chunk_index) to i64
(using try_from or as_i64-safe conversion) and increment the counter after
assigning so RawRelation.order_index is globally sequential across lines.
- Around line 265-271: The log at the start of parse_document currently emits
the raw user-supplied title at info! (log::info! with {title:?}) which can leak
PII and is too noisy; change this to a debug or trace level (e.g.,
debug!/trace!) and remove the raw title from the message, replacing it with a
stable prefix plus non-PII identifiers (for example a document_id or session_id
variable) and keep the existing diagnostic fields (config.model_name,
content.len(), chunks.len()); update the other similar logging site referenced
(lines ~917-924) the same way so neither location logs the full title at info
level.
In `@src/openhuman/memory/ingestion_rules.rs`:
- Around line 149-159: When inserting/updating RawEntity in the entities map, if
the new confidence is greater than the stored entry.confidence you must also
update entry.entity_type to the new entity_type so the stronger classification
wins; locate the block that creates/gets entry (using resolved_name,
entity_type, confidence and RawEntity) and, inside the if confidence >
entry.confidence branch, set entry.entity_type = entity_type.to_string() in
addition to updating entry.confidence so add_relation reads the correct,
strongest type.
- Around line 43-46: The rule for the "USES"|"KEEPS"|"ADOPTS" RelationRule
currently only permits ORG_TYPES as allowed_head, which causes PERSON -> uses ->
object extractions to be dropped; update the RelationRule for the branch with
canonical "USES" (the case handling "USES" | "KEEPS" | "ADOPTS") to include
PERSON_TYPES in allowed_head (e.g., ORG_TYPES | PERSON_TYPES) so person-scoped
extraction paths emitted by the parser (see ingestion_parse behavior emitting
PERSON -> uses -> object) are validated and retained.
- Around line 38-42: The pattern that collapses "WORKS_ON", "RESPONSIBLE_FOR",
and "REVIEWS" into a single RelationRule with canonical "OWNS" must be changed
so each predicate preserves its own canonical name; update the RelationRule
table (the match/arm that currently lists "OWNS" | "WORKS_ON" |
"RESPONSIBLE_FOR" | "REVIEWS") to provide separate entries (or set canonical
dynamically) so "WORKS_ON" -> canonical "WORKS_ON", "RESPONSIBLE_FOR" ->
canonical "RESPONSIBLE_FOR", and "REVIEWS" -> canonical "REVIEWS" while keeping
allowed_head/allowed_tail as appropriate; ensure this change flows into
graph_fact_regex() and the will_review handling so facts are persisted with
their original predicate names rather than "OWNS".
In `@src/openhuman/memory/ingestion_types.rs`:
- Around line 40-42: MemoryIngestionConfig currently requires model_name so
partial serde payloads fail; add a serde default for that field so missing
model_name falls back to "heuristic-only". Concretely, add a helper fn like
default_model_name() -> String { "heuristic-only".to_string() } and annotate the
model_name field with #[serde(default = "default_model_name")] in the
MemoryIngestionConfig struct (and do the same for the other identical
struct/definition around lines 76-85 or any duplicate
MemoryIngestionRequest.config usage). This preserves type (String) and ensures
deserialization of partial config payloads uses the heuristic-only default.
In `@src/openhuman/providers/compatible_stream.rs`:
- Around line 54-56: In the while loop inside compatible_stream.rs (the code
that uses buffer.find('\n')), remove the second mutation that slices buffer
after calling buffer.drain(..=pos) — specifically delete the line that does
buffer = buffer[pos + 1..].to_string(); because buffer.drain(..=pos) already
removed the consumed bytes; keep using the drained result assigned to line and
continue processing so you don't re-slice an already-mutated buffer (this fixes
the panic/data-loss in that loop).
In `@src/openhuman/skills/ops_install.rs`:
- Around line 518-559: derive_install_slug currently maps every non-alphanumeric
char to '-', which differs from slugify_skill_name in ops_create.rs that only
maps '-', '_', and whitespace to hyphens and drops other special characters;
update derive_install_slug to match slugify_skill_name’s logic: treat '-' '_'
and any whitespace as hyphen, drop all other non-alphanumeric characters
(instead of replacing them with '-'), preserve the existing lowercasing,
trimming of trailing dashes, length and forbidden-character checks, and ensure
you reference derive_install_slug and slugify_skill_name when making the change
so both functions produce consistent slugs.
- Around line 726-740: The is_private_v6 function misses IPv4-mapped IPv6
addresses (e.g. ::ffff:127.0.0.1); update is_private_v6 to detect mapped IPv4s
by calling to_ipv4_mapped() on the Ipv6Addr and, if Some(v4), return true when
is_private_v4(&v4) is true (use the same pattern as
url_guard.rs:is_non_global_v6: v6.to_ipv4_mapped().is_some_and(|v4|
is_private_v4(&v4))). Add this check before the final `false` return so
mapped-IPv4 private addresses are treated as private.
In `@src/openhuman/tools/impl/browser/native_backend.rs`:
- Around line 139-163: The handler for BrowserAction::Screenshot currently
includes the full_page flag in the response JSON but never uses it when
capturing the image (the code always calls client.screenshot()), so full-page
screenshots are not actually produced; update the BrowserAction::Screenshot
branch to honor full_page: if full_page is true, query the page height via the
WebDriver (e.g. execute_script to get document.documentElement.scrollHeight or
body.scrollHeight), adjust the viewport/window size via the WebDriver (e.g.
set_window_rect or equivalent) to that height, call client.screenshot() to
capture the full page, then restore the original viewport size; alternatively,
if implementing resizing is not possible, explicitly document the limitation by
adding a clear note to the payload or logs that full_page is unsupported and
leave the existing client.screenshot() call for viewport-only captures.
In `@src/openhuman/tools/impl/browser/security.rs`:
- Around line 94-113: The documentation-range checks in is_non_global_v4 are too
broad because they only inspect octets a and b; update the conditions that
target 192.0.2.0/24, 198.51.100.0/24, and 203.0.113.0/24 to also check the third
octet (c) from v4.octets() so they match c == 2, c == 100, and c == 113
respectively; locate the is_non_global_v4 function, destructure the octets into
[a, b, c, d], and replace the loose (a == 192 && b == 0) / (a == 198 && b == 51)
/ (a == 203 && b == 0) conditions with (a == 192 && b == 0 && c == 2) / (a ==
198 && b == 51 && c == 100) / (a == 203 && b == 0 && c == 113).
---
Nitpick comments:
In `@src/openhuman/providers/compatible_dump.rs`:
- Around line 50-59: The model-name sanitization used to build safe_model is
duplicated in dump_prompt_if_enabled and dump_response_if_enabled; extract that
logic into a small helper function (e.g., fn sanitize_model_name(model: &str) ->
String) that applies the same char filtering (allow alphanumeric, '-', '_', '.')
and returns the sanitized String, then replace the inline map/collect blocks in
both dump_prompt_if_enabled and dump_response_if_enabled to call
sanitize_model_name(model); ensure the helper is exported/visible to both
functions and update any tests or callers if necessary.
In `@src/openhuman/skills/ops_create.rs`:
- Around line 265-305: yaml_scalar currently decides to skip quoting based on
characters only, which lets strings like "true", "false", "null", "~",
integer/float forms (e.g., "123", "1.5") be emitted unquoted and therefore
parsed as non-strings by YAML consumers; update yaml_scalar to detect and
force-quote these type-ambiguous values by adding an additional needs_quote
condition that returns true when s matches YAML booleans ("true"/"false",
case-insensitive as needed), explicit null tokens ("null", "Null", "~"), or
numeric patterns (integers, floats, scientific notation), or the tilde "~";
implement these checks using simple string comparisons and/or a compact regex
and keep the existing escaping/quoting behavior in yaml_scalar.
In `@src/openhuman/tools/impl/browser/browser.rs`:
- Around line 8-16: The module declarations use explicit #[path = "..."]
attributes unnecessarily; replace lines like #[path = "action_parser.rs"] mod
action_parser; (and similarly for native_backend, security, types) with the
idiomatic plain module declarations mod action_parser;, mod security;, mod
types; and keep the conditional attribute only for native_backend as
#[cfg(feature = "browser-native")] mod native_backend; so Rust's default
same-directory resolution is used and the cfg is preserved.
In `@src/openhuman/tools/impl/browser/security.rs`:
- Around line 36-61: The current extract_host function manually parses URL
strings and can miss cases like userinfo or unusual schemes; replace its manual
parsing with robust parsing using reqwest::Url (already used in
endpoint_reachable). Update extract_host to parse url_str.trim() via
reqwest::Url::parse, return an error mapped to anyhow::Error on parse failure,
then call host_str(), ensure it is non-empty, lowercase it and return it,
otherwise bail with "Invalid URL: no host". Keep the function signature
extract_host(url_str: &str) -> anyhow::Result<String>.
- Around line 129-134: The current allow_all_browser_domains function manually
matches several case variants and misses values like "True" or "Yes"; update it
to normalize case using eq_ignore_ascii_case (or to_lowercase) and check
canonical truthy values. Locate allow_all_browser_domains and replace the
matches! block with logic that obtains the env var
(std::env::var("OPENHUMAN_BROWSER_ALLOW_ALL").ok().as_deref()) and returns true
when the string is "1" or equals_ignore_ascii_case "true" or "yes" (e.g., s ==
"1" || s.eq_ignore_ascii_case("true") || s.eq_ignore_ascii_case("yes")). Ensure
the function still returns a bool and preserves the ok().as_deref() flow.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 950ab16a-2678-4f7b-a2dc-189f676d3f17
📒 Files selected for processing (36)
src/openhuman/channels/providers/telegram/channel.rssrc/openhuman/channels/providers/telegram/channel_core.rssrc/openhuman/channels/providers/telegram/channel_ops.rssrc/openhuman/channels/providers/telegram/channel_recv.rssrc/openhuman/channels/providers/telegram/channel_send.rssrc/openhuman/channels/providers/telegram/channel_types.rssrc/openhuman/channels/providers/telegram/mod.rssrc/openhuman/composio/providers/catalogs.rssrc/openhuman/composio/providers/catalogs_business.rssrc/openhuman/composio/providers/catalogs_google.rssrc/openhuman/composio/providers/catalogs_messaging.rssrc/openhuman/composio/providers/catalogs_productivity.rssrc/openhuman/composio/providers/catalogs_social_media.rssrc/openhuman/composio/providers/mod.rssrc/openhuman/memory/ingestion.rssrc/openhuman/memory/ingestion_parse.rssrc/openhuman/memory/ingestion_regex.rssrc/openhuman/memory/ingestion_rules.rssrc/openhuman/memory/ingestion_types.rssrc/openhuman/providers/compatible.rssrc/openhuman/providers/compatible_dump.rssrc/openhuman/providers/compatible_parse.rssrc/openhuman/providers/compatible_stream.rssrc/openhuman/providers/compatible_types.rssrc/openhuman/skills/mod.rssrc/openhuman/skills/ops.rssrc/openhuman/skills/ops_create.rssrc/openhuman/skills/ops_discover.rssrc/openhuman/skills/ops_install.rssrc/openhuman/skills/ops_parse.rssrc/openhuman/skills/ops_types.rssrc/openhuman/tools/impl/browser/action_parser.rssrc/openhuman/tools/impl/browser/browser.rssrc/openhuman/tools/impl/browser/native_backend.rssrc/openhuman/tools/impl/browser/security.rssrc/openhuman/tools/impl/browser/types.rs
Comment on lines
+17
to
+23
| let pairing = if normalized_allowed.is_empty() { | ||
| let guard = PairingGuard::new(true, &[]); | ||
| if let Some(code) = guard.pairing_code() { | ||
| println!(" 🔐 Telegram pairing required. One-time bind code: {code}"); | ||
| println!(" Send `{TELEGRAM_BIND_COMMAND} <code>` from your Telegram account."); | ||
| } | ||
| Some(guard) |
Contributor
There was a problem hiding this comment.
Don't print the pairing code to stdout.
That code is effectively a bearer secret for account binding. Emitting it with println! makes it easy to capture in terminal history or centralized logs. Surface it through an explicit interactive/UI path instead of logging it verbatim.
As per coding guidelines "Never log secrets or full PII."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/channels/providers/telegram/channel_core.rs` around lines 17 -
23, The pairing code is being printed to stdout via println! in the block that
constructs a PairingGuard (see PairingGuard::new and pairing_code usage) which
exposes a bearer secret; instead, remove the println! calls and expose the code
via a secure interactive/UI pathway: have the function return the PairingGuard
(or its pairing_code) to the caller (or emit an event/callback) so the
higher-level UI can display it securely, and update any callers of this block to
handle and surface the code through the UI flow tied to TELEGRAM_BIND_COMMAND
rather than logging it to the console.
Comment on lines
+28
to
+33
| Self { | ||
| bot_token, | ||
| allowed_users: Arc::new(RwLock::new(normalized_allowed)), | ||
| pairing, | ||
| client: reqwest::Client::new(), | ||
| stream_mode: StreamMode::Off, |
Contributor
There was a problem hiding this comment.
Use one shared HTTP client configuration.
new() stores a plain reqwest::Client, but http_client() builds a fresh proxy-aware client every call. Draft/edit/delete paths use self.client, while polling/send paths use http_client(), so only part of the channel honors runtime proxy settings and none of the requests share a pool.
Suggested fix
Self {
bot_token,
allowed_users: Arc::new(RwLock::new(normalized_allowed)),
pairing,
- client: reqwest::Client::new(),
+ client: crate::openhuman::config::build_runtime_proxy_client("channel.telegram"),
stream_mode: StreamMode::Off,
draft_update_interval_ms: 1000,
silent_streaming: true,
last_draft_edit: parking_lot::Mutex::new(std::collections::HashMap::new()),
typing_handle: parking_lot::Mutex::new(None),
@@
pub(crate) fn http_client(&self) -> reqwest::Client {
- crate::openhuman::config::build_runtime_proxy_client("channel.telegram")
+ self.client.clone()
}Also applies to: 71-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/channels/providers/telegram/channel_core.rs` around lines 28 -
33, The constructor new() creates a plain reqwest::Client while http_client()
builds a proxy-aware client on each call, causing inconsistent proxy handling
and no shared connection pool; change new() to construct a single
proxy-configured reqwest::Client (using the same logic currently in
http_client()) and store it in self.client, then make http_client() return a
reference to that stored client (or remove http_client() and update users to use
self.client) so all paths (draft/edit/delete and polling/send) use the same
pooled, proxy-aware client; update references to Self::new,
ChannelCore::http_client, and any uses of self.client to ensure the shared
client is used everywhere.
Comment on lines
+8
to
+16
| pub(crate) fn typing_body_for_recipient(recipient: &str) -> serde_json::Value { | ||
| let (chat_id, thread_id) = Self::parse_reply_target(recipient); | ||
| let mut body = serde_json::json!({ | ||
| "chat_id": chat_id, | ||
| "action": "typing" | ||
| }); | ||
| if let Some(thread_id) = thread_id { | ||
| body["message_thread_id"] = serde_json::Value::String(thread_id); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Telegram Bot API: what JSON type is required for message_thread_idin methods likesendMessageandsendChatAction? Does the API accept a string, or must it be an integer?
💡 Result:
For Telegram Bot API methods like sendMessage and sendChatAction, message_thread_id is an Integer (not a string) in the API schema. So you should pass a numeric topic/thread id (e.g., 2), not JSON string (e.g., "2").
Citations:
- 1: https://core.telegram.org/bots/api
- 2: https://telegram-bot-sdk.readme.io/reference/sendchataction
- 3: https://tg-bot-sdk.website/api/methods/send-chat-action/
Serialize message_thread_id as an integer, not a JSON string.
Telegram Bot API requires message_thread_id to be an integer; the code currently serializes it as a JSON string. This will cause forum-topic requests (sendMessage, sendChatAction, etc. with thread IDs) to fail. Parse the thread ID and emit it as a numeric JSON value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/channels/providers/telegram/channel_recv.rs` around lines 8 -
16, The code in typing_body_for_recipient currently inserts thread IDs as JSON
strings, which Telegram requires to be numeric; update typing_body_for_recipient
to parse the thread_id returned by Self::parse_reply_target into an integer type
(e.g., i64 or u64) and set body["message_thread_id"] to a
serde_json::Value::Number created from that integer (instead of a String).
Ensure you handle parse errors (returning no thread_id or logging) so you only
insert a numeric serde_json::Number when parsing succeeds.
Comment on lines
+298
to
+302
| tracing::warn!( | ||
| "Telegram: ignoring message from unauthorized user: username={username}, sender_id={}. \ | ||
| Allowlist Telegram username (without '@') or numeric user ID.", | ||
| sender_id_str.as_deref().unwrap_or("unknown") | ||
| ); |
Contributor
There was a problem hiding this comment.
Redact user identifiers from warning logs.
This writes the sender's username and numeric Telegram user id verbatim. Those are full user identifiers and shouldn't be emitted to logs; use a redacted/hashable form or a correlation id instead. The same redaction should be applied to the other new Telegram sender/recipient logs in this split.
As per coding guidelines "Never log secrets or full PII."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/channels/providers/telegram/channel_recv.rs` around lines 298 -
302, The warning currently emits full PII (username and sender_id_str) in the
tracing::warn call; replace those literal identifiers with a redacted or
hashable representation (e.g., a stable HMAC/sha256 fingerprint or a correlation
id) before logging and use that redacted value in the tracing::warn message for
the current block (the tracing::warn that references username and
sender_id_str). Apply the same redaction approach to the other new Telegram
sender/recipient logs introduced in this split so no raw Telegram usernames or
numeric IDs are emitted.
Comment on lines
+215
to
+228
| let resp = self | ||
| .http_client() | ||
| .post(self.api_url(method)) | ||
| .json(&body) | ||
| .send() | ||
| .await?; | ||
|
|
||
| if !resp.status().is_success() { | ||
| let err = resp.text().await?; | ||
| anyhow::bail!("Telegram {method} by URL failed: {err}"); | ||
| } | ||
|
|
||
| tracing::info!("Telegram {method} sent to {chat_id}: {url}"); | ||
| Ok(()) |
Contributor
There was a problem hiding this comment.
Don't log attachment URLs verbatim.
These success logs include the full outbound URL. For presigned or tokenized URLs, that leaks a live secret into application logs. Log the media kind and maybe the host/basename, but redact the full URL.
As per coding guidelines "Never log secrets or full PII."
Also applies to: 599-612, 636-649
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/channels/providers/telegram/channel_send.rs` around lines 215 -
228, The success log in channel_send.rs currently logs the full outbound URL
(see the tracing::info!("Telegram {method} sent to {chat_id}: {url}") call after
the HTTP send), which may leak presigned/tokenized URLs; change that
tracing::info call (and the similar tracing::info calls referenced later) to
redact the full URL: derive and log only the media kind and a safe identifier
such as the URL host and filename/basename (or replace the URL with
"<redacted-url>") instead of the complete {url}; make this change where the code
builds requests via self.http_client(), self.api_url(method), and where
variables named url are interpolated into logs (also update the analogous
logging sites for other media-send branches noted in the review).
Comment on lines
+54
to
+56
| while let Some(pos) = buffer.find('\n') { | ||
| let line = buffer.drain(..=pos).collect::<String>(); | ||
| buffer = buffer[pos + 1..].to_string(); |
Contributor
There was a problem hiding this comment.
Bug: Double-mutation of buffer causes data loss or panic.
After buffer.drain(..=pos) mutates buffer in place (removing characters 0 through pos inclusive), line 56 incorrectly slices the already-modified buffer using the old pos index. This will either:
- Panic if
pos + 1 >= buffer.len()(out of bounds) - Skip valid characters if there's remaining content
The drain already handles the mutation correctly—line 56 should be removed.
🐛 Proposed fix
// Process complete lines
while let Some(pos) = buffer.find('\n') {
let line = buffer.drain(..=pos).collect::<String>();
- buffer = buffer[pos + 1..].to_string();
match parse_sse_line(&line) {📝 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
| while let Some(pos) = buffer.find('\n') { | |
| let line = buffer.drain(..=pos).collect::<String>(); | |
| buffer = buffer[pos + 1..].to_string(); | |
| while let Some(pos) = buffer.find('\n') { | |
| let line = buffer.drain(..=pos).collect::<String>(); | |
| match parse_sse_line(&line) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/providers/compatible_stream.rs` around lines 54 - 56, In the
while loop inside compatible_stream.rs (the code that uses buffer.find('\n')),
remove the second mutation that slices buffer after calling buffer.drain(..=pos)
— specifically delete the line that does buffer = buffer[pos + 1..].to_string();
because buffer.drain(..=pos) already removed the consumed bytes; keep using the
drained result assigned to line and continue processing so you don't re-slice an
already-mutated buffer (this fixes the panic/data-loss in that loop).
Comment on lines
+518
to
+559
| pub(crate) fn derive_install_slug(fm: &SkillFrontmatter) -> Result<String, String> { | ||
| let candidate = fm | ||
| .metadata | ||
| .get("id") | ||
| .and_then(|v| v.as_str()) | ||
| .map(|s| s.to_string()) | ||
| .unwrap_or_else(|| fm.name.clone()); | ||
|
|
||
| let mut out = String::with_capacity(candidate.len()); | ||
| let mut last_dash = false; | ||
| for ch in candidate.chars() { | ||
| if ch.is_ascii_alphanumeric() { | ||
| out.push(ch.to_ascii_lowercase()); | ||
| last_dash = false; | ||
| } else if !last_dash && !out.is_empty() { | ||
| out.push('-'); | ||
| last_dash = true; | ||
| } | ||
| } | ||
| while out.ends_with('-') { | ||
| out.pop(); | ||
| } | ||
|
|
||
| if out.is_empty() { | ||
| return Err( | ||
| "invalid SKILL.md: cannot derive slug from empty name/id — set a value in frontmatter" | ||
| .to_string(), | ||
| ); | ||
| } | ||
| if out.len() > MAX_NAME_LEN { | ||
| return Err(format!( | ||
| "invalid SKILL.md: derived slug {out:?} exceeds {MAX_NAME_LEN} chars" | ||
| )); | ||
| } | ||
| if out.contains("..") || out.contains('/') || out.contains('\\') { | ||
| return Err(format!( | ||
| "invalid SKILL.md: derived slug {out:?} contains forbidden path components" | ||
| )); | ||
| } | ||
|
|
||
| Ok(out) | ||
| } |
Contributor
There was a problem hiding this comment.
Slug derivation differs from create_skill's slugify_skill_name.
derive_install_slug converts all non-alphanumeric characters to hyphens, while slugify_skill_name in ops_create.rs only converts -, _, and whitespace to hyphens (dropping other special characters). Example:
- Input:
"My@Skill" slugify_skill_name→"myskill"(@ dropped)derive_install_slug→"my-skill"(@ → hyphen)
This inconsistency is unlikely to cause functional issues (both produce valid slugs), but could confuse users if they create a skill locally vs. install from URL with identical frontmatter names.
💡 Suggested alignment with create_skill behavior
pub(crate) fn derive_install_slug(fm: &SkillFrontmatter) -> Result<String, String> {
+ use super::ops_create::slugify_skill_name;
+
let candidate = fm
.metadata
.get("id")
.and_then(|v| v.as_str())
.map(|s| s.to_string())
.unwrap_or_else(|| fm.name.clone());
- let mut out = String::with_capacity(candidate.len());
- let mut last_dash = false;
- for ch in candidate.chars() {
- if ch.is_ascii_alphanumeric() {
- out.push(ch.to_ascii_lowercase());
- last_dash = false;
- } else if !last_dash && !out.is_empty() {
- out.push('-');
- last_dash = true;
- }
- }
- while out.ends_with('-') {
- out.pop();
- }
-
- if out.is_empty() {
- return Err(
- "invalid SKILL.md: cannot derive slug from empty name/id — set a value in frontmatter"
- .to_string(),
- );
- }
- if out.len() > MAX_NAME_LEN {
- return Err(format!(
- "invalid SKILL.md: derived slug {out:?} exceeds {MAX_NAME_LEN} chars"
- ));
- }
- if out.contains("..") || out.contains('/') || out.contains('\\') {
- return Err(format!(
- "invalid SKILL.md: derived slug {out:?} contains forbidden path components"
- ));
- }
-
- Ok(out)
+ slugify_skill_name(&candidate)
+ .map_err(|e| format!("invalid SKILL.md: {e}"))
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/skills/ops_install.rs` around lines 518 - 559,
derive_install_slug currently maps every non-alphanumeric char to '-', which
differs from slugify_skill_name in ops_create.rs that only maps '-', '_', and
whitespace to hyphens and drops other special characters; update
derive_install_slug to match slugify_skill_name’s logic: treat '-' '_' and any
whitespace as hyphen, drop all other non-alphanumeric characters (instead of
replacing them with '-'), preserve the existing lowercasing, trimming of
trailing dashes, length and forbidden-character checks, and ensure you reference
derive_install_slug and slugify_skill_name when making the change so both
functions produce consistent slugs.
Comment on lines
+726
to
+740
| fn is_private_v6(ip: &Ipv6Addr) -> bool { | ||
| if ip.is_loopback() || ip.is_unspecified() || ip.is_multicast() { | ||
| return true; | ||
| } | ||
| let first = ip.segments()[0]; | ||
| // fc00::/7 unique-local | ||
| if (first & 0xfe00) == 0xfc00 { | ||
| return true; | ||
| } | ||
| // fe80::/10 link-local | ||
| if (first & 0xffc0) == 0xfe80 { | ||
| return true; | ||
| } | ||
| false | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if IPv4-mapped IPv6 addresses could bypass SSRF protection
# Test the Rust std library behavior
cat << 'EOF' > /tmp/test_ipv6.rs
use std::net::Ipv6Addr;
fn main() {
// IPv4-mapped loopback
let mapped_loopback: Ipv6Addr = "::ffff:127.0.0.1".parse().unwrap();
println!("::ffff:127.0.0.1");
println!(" is_loopback: {}", mapped_loopback.is_loopback());
println!(" segments[0]: 0x{:04x}", mapped_loopback.segments()[0]);
// IPv4-mapped private
let mapped_private: Ipv6Addr = "::ffff:192.168.1.1".parse().unwrap();
println!("::ffff:192.168.1.1");
println!(" is_loopback: {}", mapped_private.is_loopback());
println!(" segments: {:?}", mapped_private.segments());
// Check to_ipv4_mapped (stable since 1.63)
if let Some(v4) = mapped_loopback.to_ipv4_mapped() {
println!(" to_ipv4_mapped: {}", v4);
println!(" v4.is_loopback: {}", v4.is_loopback());
}
}
EOF
# Note: This requires rustc to be available
echo "If rustc is available, run: rustc /tmp/test_ipv6.rs -o /tmp/test_ipv6 && /tmp/test_ipv6"
echo ""
echo "Expected: IPv4-mapped addresses are NOT caught by current is_private_v6 implementation"Repository: tinyhumansai/openhuman
Length of output: 242
🏁 Script executed:
# First, let's find and examine the ops_install.rs file
fd -type f -name "ops_install.rs" | head -20Repository: tinyhumansai/openhuman
Length of output: 237
🏁 Script executed:
# Get the full context of the SSRF protection functions
git ls-files | grep -E "ops_install\.rs$"Repository: tinyhumansai/openhuman
Length of output: 102
🏁 Script executed:
# Read the relevant section around lines 726-740
sed -n '680,745p' src/openhuman/skills/ops_install.rsRepository: tinyhumansai/openhuman
Length of output: 1598
🏁 Script executed:
# Also check the is_blocked_install_host function to understand the full flow
sed -n '684,750p' src/openhuman/skills/ops_install.rsRepository: tinyhumansai/openhuman
Length of output: 1578
🏁 Script executed:
# Look for where these functions are called to understand the URL parsing flow
rg -A 5 -B 5 "is_private_v6\|is_private_v4\|is_blocked_install_host" src/openhuman/skills/ops_install.rs | head -100Repository: tinyhumansai/openhuman
Length of output: 48
🏁 Script executed:
# Find calls to is_blocked_install_host to understand the URL parsing flow
rg "is_blocked_install_host" src/openhuman/skills/ops_install.rs -B 10 -A 3Repository: tinyhumansai/openhuman
Length of output: 983
🏁 Script executed:
# Check Cargo.toml to see what url crate version is used
find . -name "Cargo.toml" -exec grep -l "url" {} \; | head -5Repository: tinyhumansai/openhuman
Length of output: 106
🏁 Script executed:
# Check if there's any URL parsing or validation before is_blocked_install_host
sed -n '600,683p' src/openhuman/skills/ops_install.rsRepository: tinyhumansai/openhuman
Length of output: 3250
🏁 Script executed:
# Check Rust documentation for IPv6 address handling
# Let's see if there's any IPv4-mapped address detection in the codebase
rg "to_ipv4_mapped|ipv4_mapped" src/Repository: tinyhumansai/openhuman
Length of output: 360
🏁 Script executed:
# Let's check what Rust version is being used
grep -A 5 "^\[package\]" Cargo.toml | head -20Repository: tinyhumansai/openhuman
Length of output: 209
🏁 Script executed:
# Create a test to validate our concern about IPv4-mapped addresses
cat > /tmp/test_ipv6_mapped.py << 'EOF'
# Simulate the behavior of Rust's Ipv6Addr for IPv4-mapped addresses
# IPv4-mapped addresses have the format ::ffff:x.x.x.x
def check_ipv6(addr_str):
"""Simulate checking IPv6 address against is_private_v6 logic"""
# Parse as segments (simplified - just checking first segment)
if addr_str == "::ffff:127.0.0.1":
# This would parse as Ipv6Addr with segments [0, 0, 0, 0, 0, 0xffff, 0x7f00, 0x0001]
# First segment is 0x0000
first = 0x0000
print(f"{addr_str}: first_segment=0x{first:04x}")
print(f" is_loopback (::1): False")
print(f" is_unspecified (::): False")
print(f" is_multicast: False")
print(f" ULA fc00::/7: (0x{first:04x} & 0xfe00) == 0xfc00? {(first & 0xfe00) == 0xfc00}")
print(f" link-local fe80::/10: (0x{first:04x} & 0xffc0) == 0xfe80? {(first & 0xffc0) == 0xfe80}")
print(f" RESULT: Would be allowed (BUG!)\n")
if addr_str == "::ffff:192.168.1.1":
first = 0x0000
print(f"{addr_str}: first_segment=0x{first:04x}")
print(f" ULA fc00::/7: {(first & 0xfe00) == 0xfc00}")
print(f" link-local fe80::/10: {(first & 0xffc0) == 0xfe80}")
print(f" RESULT: Would be allowed (BUG!)\n")
if addr_str == "::1":
print(f"{addr_str}: is_loopback=True, RESULT: Correctly blocked\n")
if addr_str == "::ffff:8.8.8.8":
first = 0x0000
print(f"{addr_str}: first_segment=0x{first:04x}")
print(f" ULA fc00::/7: {(first & 0xfe00) == 0xfc00}")
print(f" link-local fe80::/10: {(first & 0xffc0) == 0xfe80}")
print(f" RESULT: Would be allowed (legitimate - public DNS)\n")
check_ipv6("::1")
check_ipv6("::ffff:127.0.0.1")
check_ipv6("::ffff:192.168.1.1")
check_ipv6("::ffff:8.8.8.8")
EOF
python /tmp/test_ipv6_mapped.pyRepository: tinyhumansai/openhuman
Length of output: 649
🏁 Script executed:
# Check how other parts of the codebase handle IPv4-mapped IPv6
cat src/openhuman/tools/impl/network/url_guard.rsRepository: tinyhumansai/openhuman
Length of output: 13987
🏁 Script executed:
# Check the browser security implementation
sed -n '1,100p' src/openhuman/tools/impl/browser/security.rs | grep -A 10 -B 10 "to_ipv4_mapped"Repository: tinyhumansai/openhuman
Length of output: 48
🏁 Script executed:
# Look for the test mentioned in url_guard.rs
rg "blocks_ipv4_mapped_ipv6" -A 20 src/Repository: tinyhumansai/openhuman
Length of output: 1718
Add IPv4-mapped IPv6 address handling to is_private_v6.
The function misses IPv4-mapped IPv6 addresses like ::ffff:127.0.0.1 and ::ffff:192.168.1.1. These parse as Ipv6Addr with first segment 0x0000, which don't match the ULA (fc00::/7) or link-local (fe80::/10) masks, allowing private addresses through.
Use the same pattern as src/openhuman/tools/impl/network/url_guard.rs:is_non_global_v6:
v6.to_ipv4_mapped().is_some_and(|v4| is_private_v4(&v4))Add this check to is_private_v6 before returning false.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/skills/ops_install.rs` around lines 726 - 740, The
is_private_v6 function misses IPv4-mapped IPv6 addresses (e.g.
::ffff:127.0.0.1); update is_private_v6 to detect mapped IPv4s by calling
to_ipv4_mapped() on the Ipv6Addr and, if Some(v4), return true when
is_private_v4(&v4) is true (use the same pattern as
url_guard.rs:is_non_global_v6: v6.to_ipv4_mapped().is_some_and(|v4|
is_private_v4(&v4))). Add this check before the final `false` return so
mapped-IPv4 private addresses are treated as private.
Comment on lines
+139
to
+163
| BrowserAction::Screenshot { path, full_page } => { | ||
| let client = self.active_client()?; | ||
| let png = client | ||
| .screenshot() | ||
| .await | ||
| .context("Failed to capture screenshot")?; | ||
| let mut payload = json!({ | ||
| "backend": "rust_native", | ||
| "action": "screenshot", | ||
| "full_page": full_page, | ||
| "bytes": png.len(), | ||
| }); | ||
|
|
||
| if let Some(path_str) = path { | ||
| tokio::fs::write(&path_str, &png) | ||
| .await | ||
| .with_context(|| format!("Failed to write screenshot to {path_str}"))?; | ||
| payload["path"] = Value::String(path_str); | ||
| } else { | ||
| payload["png_base64"] = | ||
| Value::String(base64::engine::general_purpose::STANDARD.encode(&png)); | ||
| } | ||
|
|
||
| Ok(payload) | ||
| } |
Contributor
There was a problem hiding this comment.
Screenshot full_page flag is unused.
The full_page parameter is included in the response JSON but not used in the actual screenshot capture. Fantoccini's screenshot() captures the viewport, not full page. Consider documenting this limitation or implementing full-page capture if needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/tools/impl/browser/native_backend.rs` around lines 139 - 163,
The handler for BrowserAction::Screenshot currently includes the full_page flag
in the response JSON but never uses it when capturing the image (the code always
calls client.screenshot()), so full-page screenshots are not actually produced;
update the BrowserAction::Screenshot branch to honor full_page: if full_page is
true, query the page height via the WebDriver (e.g. execute_script to get
document.documentElement.scrollHeight or body.scrollHeight), adjust the
viewport/window size via the WebDriver (e.g. set_window_rect or equivalent) to
that height, call client.screenshot() to capture the full page, then restore the
original viewport size; alternatively, if implementing resizing is not possible,
explicitly document the limitation by adding a clear note to the payload or logs
that full_page is unsupported and leave the existing client.screenshot() call
for viewport-only captures.
Comment on lines
+94
to
+113
| /// Returns `true` for any IPv4 address that is not globally routable. | ||
| pub(crate) fn is_non_global_v4(v4: std::net::Ipv4Addr) -> bool { | ||
| let [a, b, _, _] = v4.octets(); | ||
| v4.is_loopback() | ||
| || v4.is_private() | ||
| || v4.is_link_local() | ||
| || v4.is_unspecified() | ||
| || v4.is_broadcast() | ||
| || v4.is_multicast() | ||
| // Shared address space (100.64/10) | ||
| || (a == 100 && (64..=127).contains(&b)) | ||
| // Reserved (240.0.0.0/4) | ||
| || a >= 240 | ||
| // Documentation (192.0.2.0/24, 198.51.100.0/24, 203.0.113.0/24) | ||
| || (a == 192 && b == 0) | ||
| || (a == 198 && b == 51) | ||
| || (a == 203 && b == 0) | ||
| // Benchmarking (198.18.0.0/15) | ||
| || (a == 198 && (18..=19).contains(&b)) | ||
| } |
Contributor
There was a problem hiding this comment.
IPv4 documentation ranges check is incomplete.
The documentation range checks on lines 108-110 are incomplete:
192.0.2.0/24requires checking third octet (c == 2)198.51.100.0/24requires checking third octet (c == 100)203.0.113.0/24requires checking third octet (c == 113)
Currently, 192.0.0.0/16, 198.51.0.0/16, and 203.0.0.0/16 are blocked—much broader than intended.
🔒 Proposed fix for accurate documentation range checks
pub(crate) fn is_non_global_v4(v4: std::net::Ipv4Addr) -> bool {
- let [a, b, _, _] = v4.octets();
+ let [a, b, c, _] = v4.octets();
v4.is_loopback()
|| v4.is_private()
|| v4.is_link_local()
|| v4.is_unspecified()
|| v4.is_broadcast()
|| v4.is_multicast()
// Shared address space (100.64/10)
|| (a == 100 && (64..=127).contains(&b))
// Reserved (240.0.0.0/4)
|| a >= 240
// Documentation (192.0.2.0/24, 198.51.100.0/24, 203.0.113.0/24)
- || (a == 192 && b == 0)
- || (a == 198 && b == 51)
- || (a == 203 && b == 0)
+ || (a == 192 && b == 0 && c == 2)
+ || (a == 198 && b == 51 && c == 100)
+ || (a == 203 && b == 0 && c == 113)
// Benchmarking (198.18.0.0/15)
|| (a == 198 && (18..=19).contains(&b))
}📝 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
| /// Returns `true` for any IPv4 address that is not globally routable. | |
| pub(crate) fn is_non_global_v4(v4: std::net::Ipv4Addr) -> bool { | |
| let [a, b, _, _] = v4.octets(); | |
| v4.is_loopback() | |
| || v4.is_private() | |
| || v4.is_link_local() | |
| || v4.is_unspecified() | |
| || v4.is_broadcast() | |
| || v4.is_multicast() | |
| // Shared address space (100.64/10) | |
| || (a == 100 && (64..=127).contains(&b)) | |
| // Reserved (240.0.0.0/4) | |
| || a >= 240 | |
| // Documentation (192.0.2.0/24, 198.51.100.0/24, 203.0.113.0/24) | |
| || (a == 192 && b == 0) | |
| || (a == 198 && b == 51) | |
| || (a == 203 && b == 0) | |
| // Benchmarking (198.18.0.0/15) | |
| || (a == 198 && (18..=19).contains(&b)) | |
| } | |
| pub(crate) fn is_non_global_v4(v4: std::net::Ipv4Addr) -> bool { | |
| let [a, b, c, _] = v4.octets(); | |
| v4.is_loopback() | |
| || v4.is_private() | |
| || v4.is_link_local() | |
| || v4.is_unspecified() | |
| || v4.is_broadcast() | |
| || v4.is_multicast() | |
| // Shared address space (100.64/10) | |
| || (a == 100 && (64..=127).contains(&b)) | |
| // Reserved (240.0.0.0/4) | |
| || a >= 240 | |
| // Documentation (192.0.2.0/24, 198.51.100.0/24, 203.0.113.0/24) | |
| || (a == 192 && b == 0 && c == 2) | |
| || (a == 198 && b == 51 && c == 100) | |
| || (a == 203 && b == 0 && c == 113) | |
| // Benchmarking (198.18.0.0/15) | |
| || (a == 198 && (18..=19).contains(&b)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/tools/impl/browser/security.rs` around lines 94 - 113, The
documentation-range checks in is_non_global_v4 are too broad because they only
inspect octets a and b; update the conditions that target 192.0.2.0/24,
198.51.100.0/24, and 203.0.113.0/24 to also check the third octet (c) from
v4.octets() so they match c == 2, c == 100, and c == 113 respectively; locate
the is_non_global_v4 function, destructure the octets into [a, b, c, d], and
replace the loose (a == 192 && b == 0) / (a == 198 && b == 51) / (a == 203 && b
== 0) conditions with (a == 192 && b == 0 && c == 2) / (a == 198 && b == 51 && c
== 100) / (a == 203 && b == 0 && c == 113).
senamakel
approved these changes
Apr 29, 2026
2 of 7 tasks
AusAgentSmith
pushed a commit
to AusAgentSmith/openhuman
that referenced
this pull request
May 23, 2026
) (tinyhumansai#1010) Co-authored-by: Jwalin Shah <jshah1331@gmail.com>
senamakel
pushed a commit
to nocstah/openhuman
that referenced
this pull request
Sep 11, 2026
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
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
_core,_ops,_types,_parse,_send/_recv,_rules)channel.rs→channel_core,channel_ops,channel_recv,channel_send,channel_typescatalogs.rs→catalogs_business,catalogs_google,catalogs_messaging,catalogs_productivity,catalogs_social_mediaingestion.rs→ingestion_parse,ingestion_regex,ingestion_rules,ingestion_typescompatible.rs→compatible_dump,compatible_parse,compatible_stream,compatible_typesops.rs→ops_create,ops_discover,ops_install,ops_parse,ops_typesaction_parser,native_backend,security,typesmod.rspreserve all public interfaces unchangedmainafter branch cut:Skill.dir_name,Skill.read_body(),uninstall_skill/UninstallSkillParams,TelegramChannel.silent_streamingTest plan
cargo checkpasses (0 errors)cargo fmt --checkpassescargo test)pnpm typecheckandpnpm lintunaffectedSummary by CodeRabbit
New Features
Refactoring