Trim Rust dependency graph and duplicate TLS stacks - #5202
Conversation
This reverts commit 6721f745c9ab1a60ffea4069ad4debd8baab8d94.
📝 WalkthroughWalkthroughThis PR adds Linux TLS dependency checks, a bounded asynchronous Sentry transport, and a vendored OAuth crate with PKCE, local callbacks, token exchange, and Codex provider support. ChangesRust dependency and TLS policy
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant OAuthLogin
participant BoundServer
participant Browser
participant TokenEndpoint
OAuthLogin->>BoundServer: bind callback listener
OAuthLogin->>Browser: open authorization URL
Browser->>BoundServer: redirect with code and state
BoundServer-->>OAuthLogin: return callback parameters
OAuthLogin->>TokenEndpoint: exchange authorization code
TokenEndpoint-->>OAuthLogin: return token response
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
coderabbitai
Bot
added
bug
rust-core
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86cfcf1956
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| match client | ||
| .post(&url) | ||
| .header("X-Sentry-Auth", &auth) | ||
| .body(body) | ||
| .send() |
There was a problem hiding this comment.
When the Sentry endpoint accepts a connection but stalls without returning a response, this blocking client has no request timeout, so the worker can remain stuck in send(). Although flush(timeout) eventually reports failure, Drop then unconditionally joins that worker, causing the CLI or desktop process to hang during shutdown rather than respecting Sentry's shutdown timeout; configure a bounded request timeout or avoid an unbounded join.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
vendor/motosan-ai-oauth/src/lib.rs (1)
65-102: 📐 Maintainability & Code Quality | 🔵 TrivialNo structured diagnostics on the login/exchange flow.
loginhas nolog::calls for entry/exit, the browser-open step, or the callback wait/timeout — only aprintln!. As per coding guidelines, "New or changed flows must include verbose, grep-friendly diagnostics covering entry/exit, branches, external calls, retries/timeouts, state transitions, and errors." That said, README.openhuman.md scopes this vendored crate's patch to the manifest only, so adding logging here diverges from upstream; consider whether the consuming app (src/openhuman/inference/openai_oauth/*) already covers this instead.🤖 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 `@vendor/motosan-ai-oauth/src/lib.rs` around lines 65 - 102, The login flow in login lacks structured diagnostics, but this vendored crate is scoped to manifest-only changes. Check the consuming OAuth integration under the existing OpenAI OAuth symbols and add grep-friendly entry/exit, browser-open, callback wait/timeout, state-transition, exchange, and error diagnostics there instead; leave login unchanged unless the project’s documented scope explicitly permits modifying the vendored crate.Source: Coding guidelines
🤖 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 @.github/workflows/ci-lite.yml:
- Around line 154-155: Add the vendor/motosan-ai-oauth/** path to the rust-tauri
filter in the CI workflow, alongside its existing filter entries, so changes to
the patched vendored OAuth crate trigger the Tauri lane.
In `@scripts/check-linux-tls-dependencies.sh`:
- Around line 27-53: Normalize both reverse cargo tree queries in the package
loop by adding --prefix none to the cargo tree invocations used by the owners
variable. Keep the existing Sentry and motosan-ai-oauth ownership checks
unchanged so their anchored grep patterns reliably match emitted owner lines.
In `@src/core/sentry_transport.rs`:
- Around line 56-132: Make SharedReqwestTransport diagnostics visible through
the application’s standard logging pipeline, not only sentry_debug!. Add
matching log/tracing messages for envelope drops, rate-limit discards,
serialization failures, send failures, and transport setup/proxy errors, using
appropriate severity and grep-friendly context. Handle thread spawn failure
instead of silently converting it with .ok(), logging the error so subsequent
send failures remain diagnosable.
- Around line 24-45: Update SharedReqwestTransport::new to configure a finite
reqwest client timeout before building the client, preventing send() and worker
shutdown from blocking indefinitely. Replace the builder.build().expect(...)
path with graceful Result handling that disables or skips transport creation
when client construction fails, without crashing startup; preserve existing
proxy and certificate options.
In `@vendor/motosan-ai-oauth/src/exchange.rs`:
- Around line 32-64: The post_token request currently has no timeout and can
hang indefinitely. Configure the reqwest::Client created in post_token with an
appropriate request timeout so both exchange_code and refresh_token operations
terminate when the token endpoint stalls.
In `@vendor/motosan-ai-oauth/src/lib.rs`:
- Around line 142-152: Update the Windows branch of open_browser to quote the
URL when invoking cmd /c start, ensuring URLs containing ampersands are passed
as a single argument while preserving the existing macOS and Linux behavior.
In `@vendor/motosan-ai-oauth/src/server.rs`:
- Around line 37-82: Update wait_for_callback to apply a per-connection timeout
while reading and handling each accepted stream, so stalled clients cannot block
subsequent callbacks. Isolate connection-specific failures, including
read_headers errors such as “request too large,” by discarding that connection
and continuing the accept loop instead of propagating the error and aborting the
login flow.
---
Nitpick comments:
In `@vendor/motosan-ai-oauth/src/lib.rs`:
- Around line 65-102: The login flow in login lacks structured diagnostics, but
this vendored crate is scoped to manifest-only changes. Check the consuming
OAuth integration under the existing OpenAI OAuth symbols and add grep-friendly
entry/exit, browser-open, callback wait/timeout, state-transition, exchange, and
error diagnostics there instead; leave login unchanged unless the project’s
documented scope explicitly permits modifying the vendored crate.
🪄 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: c2d52810-c1fe-4f66-9d6f-80c9c58e4a5a
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockapp/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.github/workflows/ci-lite.ymlCargo.tomlapp/src-tauri/Cargo.tomlapp/src-tauri/src/lib.rsscripts/check-linux-tls-dependencies.shsrc/core/mod.rssrc/core/sentry_transport.rssrc/main.rsvendor/motosan-ai-oauth/Cargo.tomlvendor/motosan-ai-oauth/README.openhuman.mdvendor/motosan-ai-oauth/src/error.rsvendor/motosan-ai-oauth/src/exchange.rsvendor/motosan-ai-oauth/src/lib.rsvendor/motosan-ai-oauth/src/pkce.rsvendor/motosan-ai-oauth/src/providers/codex.rsvendor/motosan-ai-oauth/src/providers/mod.rsvendor/motosan-ai-oauth/src/server.rs
| - 'vendor/motosan-ai-oauth/**' | ||
| - 'vendor/tinychannels' |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Trigger the Tauri lane for vendored OAuth changes.
app/src-tauri/Cargo.toml patches motosan-ai-oauth to ../../vendor/motosan-ai-oauth, but this path is only included in rust-core and rust-core-full. Changes to the vendored crate can therefore skip Tauri clippy/build/test coverage.
Add vendor/motosan-ai-oauth/** to the rust-tauri filter.
🤖 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 @.github/workflows/ci-lite.yml around lines 154 - 155, Add the
vendor/motosan-ai-oauth/** path to the rust-tauri filter in the CI workflow,
alongside its existing filter entries, so changes to the patched vendored OAuth
crate trigger the Tauri lane.
| owners="$( | ||
| cargo tree --locked --manifest-path "$manifest" --target "$target" \ | ||
| --invert "reqwest@$version" 2>/dev/null || true | ||
| )" | ||
| if printf '%s\n' "$owners" | grep -Eq '^sentry v'; then | ||
| printf 'error: Sentry owns reqwest %s in %s\n%s\n' \ | ||
| "$version" "$label" "$owners" >&2 | ||
| exit 1 | ||
| fi | ||
| done | ||
|
|
||
| for package in native-tls openssl openssl-sys; do | ||
| owners="$( | ||
| cargo tree --locked --manifest-path "$manifest" --target "$target" \ | ||
| --invert "$package" 2>/dev/null || true | ||
| )" | ||
| if printf '%s\n' "$owners" | grep -Eq '^sentry v'; then | ||
| printf 'error: Sentry owns %s in %s\n%s\n' \ | ||
| "$package" "$label" "$owners" >&2 | ||
| exit 1 | ||
| fi | ||
| if [[ "$label" == tauri ]] && | ||
| printf '%s\n' "$owners" | grep -Eq '^motosan-ai-oauth v'; then | ||
| printf 'error: motosan-ai-oauth owns %s in %s\n%s\n' \ | ||
| "$package" "$label" "$owners" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- scripts/check-linux-tls-dependencies.sh ---'
nl -ba scripts/check-linux-tls-dependencies.sh | sed -n '1,120p'
printf '\n%s\n' '--- cargo tree help excerpt ---'
cargo tree --help | sed -n '1,220p' | grep -nE -- '--prefix|invert|prefix none|graph-prefix|charset' || trueRepository: tinyhumansai/openhuman
Length of output: 152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- scripts/check-linux-tls-dependencies.sh: lines 20-60 ---'
sed -n '20,60p' scripts/check-linux-tls-dependencies.sh | nl -ba
echo
echo '--- cargo tree --help relevant lines ---'
cargo tree --help | grep -nE -- '--prefix|invert|graph-prefix|charset' | head -n 20Repository: tinyhumansai/openhuman
Length of output: 152
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('scripts/check-linux-tls-dependencies.sh')
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if 20 <= i <= 60:
print(f"{i:4d}: {line}")
PYRepository: tinyhumansai/openhuman
Length of output: 1780
🌐 Web query:
cargo tree --invert prefix none output reverse dependency tree documentation
💡 Result:
In the cargo tree command, the --invert (-i) and --prefix options serve distinct purposes for controlling the display of dependency graphs [1][2]. --invert (or -i) The --invert flag is used to show reverse dependencies [1][3]. Instead of displaying the tree of dependencies for your project, it inverts the graph to show which packages depend on a specified package [1][4]. When you provide a package name as a spec with --invert, cargo displays the chain of dependencies that lead to that package [4][3]. --prefix The --prefix option determines how each line in the output is displayed [2][5]. It accepts the following values: indent (default): Shows each line indented as a tree structure [2][5]. depth: Displays the output as a list with the numeric depth printed before each entry [2][5]. none: Displays the dependency information as a flat list [2][5]. Using --prefix none is particularly useful when you want to output a machine-readable, flat list of dependencies (or reverse dependencies) without the tree-like indentation characters [2][5]. You can combine these flags, for example, to get a flat list of all packages that depend on a specific package by running: cargo tree --invert --prefix none
Citations:
- 1: https://doc.rust-lang.org/stable/cargo/commands/cargo-tree.html
- 2: https://manpages.debian.org/unstable/cargo/cargo-tree.1.en.html
- 3: https://github.com/rust-lang/cargo/blob/master/doc/man/generated_txt/cargo-tree.txt
- 4: https://github.com/rust-lang/cargo/blob/master/doc/man/cargo-tree.md
- 5: https://man.archlinux.org/man/cargo-tree.1.en
Normalize the reverse cargo tree checks. --invert still emits tree prefixes by default, so grep -Eq '^sentry v' and '^motosan-ai-oauth v' can miss nested owners. Add --prefix none to both reverse queries, or loosen the match to accept prefixes.
🤖 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 `@scripts/check-linux-tls-dependencies.sh` around lines 27 - 53, Normalize both
reverse cargo tree queries in the package loop by adding --prefix none to the
cargo tree invocations used by the owners variable. Keep the existing Sentry and
motosan-ai-oauth ownership checks unchanged so their anchored grep patterns
reliably match emitted owner lines.
| impl SharedReqwestTransport { | ||
| /// Build a transport from the options consumed by Sentry's HTTP transport. | ||
| pub fn new(options: &ClientOptions) -> Self { | ||
| let mut builder = reqwest::blocking::Client::builder(); | ||
| if options.accept_invalid_certs { | ||
| builder = builder.danger_accept_invalid_certs(true); | ||
| } | ||
| if let Some(url) = options.http_proxy.as_ref() { | ||
| match reqwest::Proxy::http(url.as_ref()) { | ||
| Ok(proxy) => builder = builder.proxy(proxy), | ||
| Err(error) => sentry_debug!("invalid HTTP proxy: {error:?}"), | ||
| } | ||
| } | ||
| if let Some(url) = options.https_proxy.as_ref() { | ||
| match reqwest::Proxy::https(url.as_ref()) { | ||
| Ok(proxy) => builder = builder.proxy(proxy), | ||
| Err(error) => sentry_debug!("invalid HTTPS proxy: {error:?}"), | ||
| } | ||
| } | ||
| let client = builder | ||
| .build() | ||
| .expect("reqwest 0.12 TLS client must be available"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file outline ==\n'
ast-grep outline src/core/sentry_transport.rs --view expanded || true
printf '\n== relevant sections ==\n'
sed -n '1,220p' src/core/sentry_transport.rs | cat -n
printf '\n== dependency pins ==\n'
rg -n 'reqwest|sentry' Cargo.toml Cargo.lock -g 'Cargo.toml' -g 'Cargo.lock' || true
printf '\n== reqwest feature config candidates ==\n'
rg -n 'default-features|features\s*=|\breqwest\b' Cargo.toml -g 'Cargo.toml' || trueRepository: tinyhumansai/openhuman
Length of output: 21251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== inspect cargo manifests ==\n'
for f in $(git ls-files 'Cargo.toml' '**/Cargo.toml'); do
echo "--- $f"
sed -n '1,220p' "$f" | cat -n
doneRepository: tinyhumansai/openhuman
Length of output: 29512
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show only the reqwest/sentry dependency declarations and the transport source.
for f in $(git ls-files 'Cargo.toml' 'Cargo.lock' 'src/core/sentry_transport.rs'); do
echo "===== $f ====="
sed -n '1,220p' "$f" | cat -n
echo
doneRepository: tinyhumansai/openhuman
Length of output: 28936
🌐 Web query:
reqwest 0.12 ClientBuilder::build failure causes blocking client build error TLS backend proxy options documentation
💡 Result:
In reqwest 0.12, the ClientBuilder::build() method is designed to return a Result<Client, Error> [1][2][3]. Unlike previous versions or certain alternative constructor patterns that might panic, using Client::builder() and calling .build() allows you to handle initialization failures programmatically as a returned Error [4][5]. Causes of ClientBuilder::build() failure According to official documentation, build() will return an error in the following primary scenarios [1][2][3]: * TLS backend initialization failure: This occurs if the selected TLS backend (e.g., rustls or native-tls) cannot be initialized [1][2][3]. This often relates to environment-specific issues, such as missing system dependencies or mismatched feature configurations [6][7][8]. * System resolver configuration failure: This occurs if the client cannot load the system's DNS resolver configuration [1][2][3]. Proxy and TLS Considerations * TLS Backend Sync: If you are manually providing custom TLS configurations (e.g., use_preconfigured_tls), you must ensure that the versions of the TLS library (like rustls) used in your Cargo.toml are perfectly in sync with the version reqwest is using internally [6][8]. Mismatches often cause runtime "Unknown TLS backend" errors [6][8]. * Proxy Configuration: Proxy settings are configured via ClientBuilder::proxy() [9]. Failures during the build process are generally not specific to proxy settings unless the proxy configuration itself involves invalid URLs (which would cause an error) [4]. If a request fails later, it is typically a networking issue rather than a Client build error [4]. * Best Practices: It is highly recommended to rely on ClientBuilder methods (like add_root_certificate) rather than "advanced mode" methods like use_preconfigured_tls whenever possible to avoid backend compatibility issues [6][8]. If you encounter silent failures or unexpected behavior, ensure your code checks the Result returned by build() explicitly [10]. Note that in reqwest 0.12, ClientBuilder methods consume and return self (by-value), requiring a pattern like builder = builder.gzip(false); if configuration is conditional [11].
Citations:
- 1: https://docs.rs/reqwest/%3E=0.11,%20%3C=0.12/struct.ClientBuilder.html
- 2: https://docs.rs/reqwest/latest/reqwest/struct.ClientBuilder.html
- 3: https://docs.rs/reqwest/latest/reqwest/struct.ClientBuilder.html?search=std%3A%3Avec
- 4: https://docs.rs/reqwest/0.12.23/reqwest/struct.Client.html
- 5: https://docs.rs/reqwest/%3E=0.11,%20%3C=0.12/struct.Client.html
- 6: use_preconfigured_tls always results in "Unknown TLS backend passed to
use_preconfigured_tls" seanmonstar/reqwest#2622 - 7: https://docs.rs/reqwest/latest/src/reqwest/tls.rs.html
- 8: Unknown TLS backend passed to
use_preconfigured_tlsseanmonstar/reqwest#2171 - 9: https://users.rust-lang.org/t/reqwest-via-https-proxy-fails/135156
- 10: https://users.rust-lang.org/t/reqwest-clientbuilder-is-erroring-out-silently/127829
- 11: https://github.com/seanmonstar/reqwest/blob/v0.12.14/CHANGELOG.md
Add a timeout and avoid expect() on the reqwest client.
send()can block forever here, andDrop::drop()joins the worker with no timeout, so a stalled Sentry endpoint can hang shutdown.ClientBuilder::build()returns aResult, soexpect(...)turns TLS/resolver setup failures into a startup crash instead of disabling crash reporting.
🤖 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/core/sentry_transport.rs` around lines 24 - 45, Update
SharedReqwestTransport::new to configure a finite reqwest client timeout before
building the client, preventing send() and worker shutdown from blocking
indefinitely. Replace the builder.build().expect(...) path with graceful Result
handling that disables or skips transport creation when client construction
fails, without crashing startup; preserve existing proxy and certificate
options.
| let handle = std::thread::Builder::new() | ||
| .name("sentry-transport".into()) | ||
| .spawn(move || { | ||
| let mut rate_limiter = RateLimiter::new(); | ||
| for task in receiver { | ||
| if worker_shutdown.load(Ordering::SeqCst) { | ||
| return; | ||
| } | ||
| let envelope = match task { | ||
| Task::Send(envelope) => envelope, | ||
| Task::Flush(done) => { | ||
| let _ = done.send(()); | ||
| continue; | ||
| } | ||
| Task::Shutdown => return, | ||
| }; | ||
| if rate_limiter | ||
| .is_disabled(RateLimitingCategory::Any) | ||
| .is_some() | ||
| { | ||
| sentry_debug!("Sentry envelope discarded due to global rate limit"); | ||
| continue; | ||
| } | ||
| let Some(envelope) = rate_limiter.filter_envelope(envelope) else { | ||
| sentry_debug!("Sentry envelope discarded due to category rate limit"); | ||
| continue; | ||
| }; | ||
| let mut body = Vec::new(); | ||
| if let Err(error) = envelope.to_writer(&mut body) { | ||
| sentry_debug!("failed to serialize Sentry envelope: {error}"); | ||
| continue; | ||
| } | ||
| match client | ||
| .post(&url) | ||
| .header("X-Sentry-Auth", &auth) | ||
| .body(body) | ||
| .send() | ||
| { | ||
| Ok(response) => { | ||
| if let Some(value) = response | ||
| .headers() | ||
| .get("x-sentry-rate-limits") | ||
| .and_then(|value| value.to_str().ok()) | ||
| { | ||
| rate_limiter.update_from_sentry_header(value); | ||
| } else if let Some(value) = response | ||
| .headers() | ||
| .get(reqwest::header::RETRY_AFTER) | ||
| .and_then(|value| value.to_str().ok()) | ||
| { | ||
| rate_limiter.update_from_retry_after(value); | ||
| } else if response.status() == reqwest::StatusCode::TOO_MANY_REQUESTS { | ||
| rate_limiter.update_from_429(); | ||
| } | ||
| if response.status() == reqwest::StatusCode::PAYLOAD_TOO_LARGE { | ||
| sentry_debug!("Sentry envelope rejected as too large"); | ||
| } | ||
| } | ||
| Err(error) => sentry_debug!("failed to send Sentry envelope: {error}"), | ||
| } | ||
| } | ||
| }) | ||
| .ok(); | ||
| Self { | ||
| sender, | ||
| shutdown, | ||
| handle, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl Transport for SharedReqwestTransport { | ||
| fn send_envelope(&self, envelope: Envelope) { | ||
| if let Err(error) = self.sender.try_send(Task::Send(envelope)) { | ||
| sentry_debug!("Sentry envelope dropped: {error}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Diagnostics for this new flow are effectively invisible in production.
Every failure branch here (invalid proxy at 34/40, envelope dropped at 130, discarded due to rate limit at 76/80, serialize failure at 85, send failure at 114) logs exclusively via sentry_debug!, which per Sentry's docs only emits when ClientOptions.debug == true — off by default in production. Additionally, thread-spawn failure at line 118 (.spawn(...).ok()) is completely silent: handle becomes None and every future envelope is dropped via try_send with no diagnostic trail at all.
As per coding guidelines, **/*.{rs,ts,tsx}: "New or changed flows must include verbose, grep-friendly diagnostics covering entry/exit, branches, external calls, retries/timeouts, state transitions, and errors." This module's error paths won't surface in the project's normal log/tracing/file-logging pipeline used everywhere else (main.rs, lib.rs).
🛠️ Example fix (spawn failure + send failure)
.spawn(move || {
...
})
- .ok();
+ .map_err(|error| {
+ log::error!("[sentry-transport] failed to spawn worker thread: {error}");
+ error
+ })
+ .ok();And alongside each sentry_debug!(...) call, add a matching log::debug!/log::warn! so failures are grep-able in the app's standard log files even when ClientOptions.debug is off.
🤖 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/core/sentry_transport.rs` around lines 56 - 132, Make
SharedReqwestTransport diagnostics visible through the application’s standard
logging pipeline, not only sentry_debug!. Add matching log/tracing messages for
envelope drops, rate-limit discards, serialization failures, send failures, and
transport setup/proxy errors, using appropriate severity and grep-friendly
context. Handle thread spawn failure instead of silently converting it with
.ok(), logging the error so subsequent send failures remain diagnosable.
Source: Coding guidelines
| async fn post_token( | ||
| token_url: &str, | ||
| body_format: crate::TokenBodyFormat, | ||
| params: Vec<(&str, &str)>, | ||
| fallback_refresh: Option<&str>, | ||
| ) -> Result<Token, Error> { | ||
| let client = reqwest::Client::new(); | ||
| // No custom User-Agent override — reqwest's default is safer than a | ||
| // "Mozilla/5.0..." string (which can look like a bot to anti-abuse | ||
| // systems). Accept: application/json matches pi-ai's reference impl. | ||
| let req = client.post(token_url).header("Accept", "application/json"); | ||
|
|
||
| let req = match body_format { | ||
| crate::TokenBodyFormat::Form => req.form(¶ms), | ||
| crate::TokenBodyFormat::Json => { | ||
| let map: std::collections::HashMap<&str, &str> = params.iter().copied().collect(); | ||
| req.json(&map) | ||
| } | ||
| }; | ||
|
|
||
| let resp = req.send().await?; | ||
|
|
||
| if !resp.status().is_success() { | ||
| let status = resp.status(); | ||
| let body = resp.text().await.unwrap_or_default(); | ||
| return Err(Error::TokenExchange(format!("HTTP {status}: {body}"))); | ||
| } | ||
|
|
||
| Ok(resp | ||
| .json::<RawTokenResponse>() | ||
| .await? | ||
| .into_token(fallback_refresh)) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the function and its callers
rg -n "async fn post_token|post_token\(" vendor/motosan-ai-oauth/src src -S
# Inspect the exchange and refresh paths
sed -n '1,220p' vendor/motosan-ai-oauth/src/exchange.rs
printf '\n---\n'
sed -n '1,240p' src/openhuman/inference/openai_oauth/store.rs
printf '\n---\n'
sed -n '1,220p' vendor/motosan-ai-oauth/src/lib.rsRepository: tinyhumansai/openhuman
Length of output: 23398
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check reqwest timeout defaults in local docs/source references if present
rg -n "timeout\(" Cargo.lock Cargo.toml vendor -S | head -n 80Repository: tinyhumansai/openhuman
Length of output: 253
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find refresh-token call sites and inspect the surrounding logic
rg -n "refresh_token\\(|refresh\\(" src vendor/motosan-ai-oauth/src -S
printf '\n--- store refresh context ---\n'
sed -n '240,520p' src/openhuman/inference/openai_oauth/store.rs
printf '\n--- oauth provider context ---\n'
sed -n '1,260p' src/openhuman/inference/openai_oauth.rsRepository: tinyhumansai/openhuman
Length of output: 6009
Add a timeout to token exchange requests. post_token creates a fresh reqwest::Client with no request timeout, so a stalled token endpoint can hang exchange_code or refresh_token indefinitely; the refresh path is also driven from lookup_openai_oauth_credentials without any caller-side timeout.
🤖 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 `@vendor/motosan-ai-oauth/src/exchange.rs` around lines 32 - 64, The post_token
request currently has no timeout and can hang indefinitely. Configure the
reqwest::Client created in post_token with an appropriate request timeout so
both exchange_code and refresh_token operations terminate when the token
endpoint stalls.
| fn open_browser(url: &str) -> std::io::Result<()> { | ||
| #[cfg(target_os = "macos")] | ||
| std::process::Command::new("open").arg(url).spawn()?; | ||
| #[cfg(target_os = "linux")] | ||
| std::process::Command::new("xdg-open").arg(url).spawn()?; | ||
| #[cfg(target_os = "windows")] | ||
| std::process::Command::new("cmd") | ||
| .args(["/c", "start", "", url]) | ||
| .spawn()?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- vendor/motosan-ai-oauth/src/lib.rs (relevant sections) ---'
sed -n '100,180p' vendor/motosan-ai-oauth/src/lib.rs | cat -n
echo '--- vendor/motosan-ai-oauth/src/providers/codex.rs (relevant sections) ---'
sed -n '1,220p' vendor/motosan-ai-oauth/src/providers/codex.rs | cat -nRepository: tinyhumansai/openhuman
Length of output: 4956
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('vendor/motosan-ai-oauth/src/lib.rs')
text = p.read_text()
for needle in ['build_auth_url', 'open_browser', 'extra_auth_params', 'query_pairs_mut']:
print(f'--- matches for {needle} ---')
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
print(f'{i}: {line}')
PYRepository: tinyhumansai/openhuman
Length of output: 1757
🌐 Web query:
cmd.exe start bare ampersand URL command separator unescaped & breaks start command documentation
💡 Result:
When using the Windows start command, the ampersand (&) is treated as a command separator, which causes it to break the command line if it appears unescaped in a URL or file path [1][2][3]. The command processor interprets everything following the unescaped & as a separate command to be executed, rather than part of the intended target [4][3]. To resolve this, you can use one of the following two primary methods: 1. Enclose in Quotes with a Title Argument: The start command expects its first argument to be the title of the new window [5]. If you are passing a URL or path that contains special characters like &, you can provide an empty title argument ("") followed by the quoted URL or path [6][7]. This treats the contents of the quoted string as a single argument: start "" "https://example.com/page?query=1&id=2" 2. Escape with the Caret (^): You can escape each individual ampersand by prefixing it with the caret character (^). This informs the command processor to treat the & as a literal character rather than a special command separator [1][2][6]. start https://example.com/page?query=1^&id=2 If you are attempting to run multiple commands within the new window opened by start, you must quote the entire string and often use the cmd /k or /c switches to ensure the command processor interprets the sequence correctly as a single unit [4][8]. Note: For complex paths, especially those involving environment variables or deeply nested quotes, consistently using the empty title approach ("") combined with proper quoting is generally the most robust solution [6][7][9].
Citations:
- 1: https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/cmd
- 2: https://github.com/johnstevenson/winbox-args/wiki/How-cmd.exe-parses-a-command
- 3: https://ourtechroom.com/tech/open-ampersand-containing-url-with-start-command/
- 4: https://superuser.com/questions/1680186/how-do-i-pass-multiple-commands-to-start-in-windows-command-line
- 5: https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/start
- 6: https://stackoverflow.com/questions/1327431/how-do-i-escape-ampersands-in-batch-files
- 7: https://stackoverflow.com/questions/9415689/windows-batch-file-start-command-is-not-opening-complete-url
- 8: https://stackoverflow.com/questions/48199915/passing-multiple-commands-to-start-command-in-batch-file
- 9: https://stackoverflow.com/questions/44513063/how-to-open-a-url-that-contains-an-ampersand-with-start-command
Quote the URL in the Windows launcher. cmd /c start reparses & as a separator, so passing the OAuth URL directly can break browser launch on Windows. Use start "" "<url>" (or escape &) here.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 147-148: Passing non-literal (user-controlled or interpolated) data to a shell invoked via std::process::Command::new("sh"|"bash"|...) with -c allows command injection. Avoid spawning a shell: pass the program and each argument separately to Command::new(program).arg(arg) so the OS never re-parses the string, or strictly allowlist/escape any value that must reach a shell.
Context: std::process::Command::new("cmd")
.args(["/c", "start", "", url])
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-process-shell-rust)
🤖 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 `@vendor/motosan-ai-oauth/src/lib.rs` around lines 142 - 152, Update the
Windows branch of open_browser to quote the URL when invoking cmd /c start,
ensuring URLs containing ampersands are passed as a single argument while
preserving the existing macOS and Linux behavior.
| pub async fn wait_for_callback( | ||
| server: BoundServer, | ||
| callback_path: &str, | ||
| ) -> Result<(String, String), Error> { | ||
| let BoundServer { listener, .. } = server; | ||
| loop { | ||
| let (mut stream, _) = listener.accept().await?; | ||
| let buf = read_headers(&mut stream).await?; | ||
| let request = String::from_utf8_lossy(&buf); | ||
|
|
||
| if !is_callback_request(&request, callback_path) { | ||
| let _ = stream | ||
| .write_all(b"HTTP/1.1 204 No Content\r\nConnection: close\r\n\r\n") | ||
| .await; | ||
| continue; | ||
| } | ||
|
|
||
| let html = "<html><body>Login successful. You can close this tab.</body></html>"; | ||
| let response = format!( | ||
| "HTTP/1.1 200 OK\r\nContent-Type: text/html\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{}", | ||
| html.len(), | ||
| html | ||
| ); | ||
| let _ = stream.write_all(response.as_bytes()).await; | ||
| return parse_callback(&request); | ||
| } | ||
| } | ||
|
|
||
| async fn read_headers(stream: &mut tokio::net::TcpStream) -> Result<Vec<u8>, Error> { | ||
| let mut buf = Vec::with_capacity(4096); | ||
| let mut chunk = [0u8; 512]; | ||
| loop { | ||
| let n = stream.read(&mut chunk).await?; | ||
| if n == 0 { | ||
| break; | ||
| } | ||
| if buf.len() + n >= 16384 { | ||
| return Err(Error::Callback("request too large".into())); | ||
| } | ||
| buf.extend_from_slice(&chunk[..n]); | ||
| if buf.windows(4).any(|w| w == b"\r\n\r\n") { | ||
| break; | ||
| } | ||
| } | ||
| Ok(buf) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Single stalled/oversized connection can block the whole login flow.
wait_for_callback handles one connection at a time with no per-connection timeout; a stray or malicious local connection to this port (prefetch probe, other local process, port scan) can occupy the loop for the entire 120s window before the real browser callback is even accepted. Additionally, read_headers's "request too large" error propagates via ? and aborts the whole login attempt rather than just discarding that one bad connection.
⏱️ Proposed fix: bound and isolate per-connection handling
loop {
let (mut stream, _) = listener.accept().await?;
- let buf = read_headers(&mut stream).await?;
+ let buf = match tokio::time::timeout(
+ std::time::Duration::from_secs(10),
+ read_headers(&mut stream),
+ )
+ .await
+ {
+ Ok(Ok(buf)) => buf,
+ // Drop slow, oversized, or malformed connections and keep listening
+ // for the real callback instead of aborting the whole login.
+ Ok(Err(_)) | Err(_) => continue,
+ };
let request = String::from_utf8_lossy(&buf);📝 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.
| pub async fn wait_for_callback( | |
| server: BoundServer, | |
| callback_path: &str, | |
| ) -> Result<(String, String), Error> { | |
| let BoundServer { listener, .. } = server; | |
| loop { | |
| let (mut stream, _) = listener.accept().await?; | |
| let buf = read_headers(&mut stream).await?; | |
| let request = String::from_utf8_lossy(&buf); | |
| if !is_callback_request(&request, callback_path) { | |
| let _ = stream | |
| .write_all(b"HTTP/1.1 204 No Content\r\nConnection: close\r\n\r\n") | |
| .await; | |
| continue; | |
| } | |
| let html = "<html><body>Login successful. You can close this tab.</body></html>"; | |
| let response = format!( | |
| "HTTP/1.1 200 OK\r\nContent-Type: text/html\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{}", | |
| html.len(), | |
| html | |
| ); | |
| let _ = stream.write_all(response.as_bytes()).await; | |
| return parse_callback(&request); | |
| } | |
| } | |
| async fn read_headers(stream: &mut tokio::net::TcpStream) -> Result<Vec<u8>, Error> { | |
| let mut buf = Vec::with_capacity(4096); | |
| let mut chunk = [0u8; 512]; | |
| loop { | |
| let n = stream.read(&mut chunk).await?; | |
| if n == 0 { | |
| break; | |
| } | |
| if buf.len() + n >= 16384 { | |
| return Err(Error::Callback("request too large".into())); | |
| } | |
| buf.extend_from_slice(&chunk[..n]); | |
| if buf.windows(4).any(|w| w == b"\r\n\r\n") { | |
| break; | |
| } | |
| } | |
| Ok(buf) | |
| } | |
| pub async fn wait_for_callback( | |
| server: BoundServer, | |
| callback_path: &str, | |
| ) -> Result<(String, String), Error> { | |
| let BoundServer { listener, .. } = server; | |
| loop { | |
| let (mut stream, _) = listener.accept().await?; | |
| let buf = match tokio::time::timeout( | |
| std::time::Duration::from_secs(10), | |
| read_headers(&mut stream), | |
| ) | |
| .await | |
| { | |
| Ok(Ok(buf)) => buf, | |
| // Drop slow, oversized, or malformed connections and keep listening | |
| // for the real callback instead of aborting the whole login. | |
| Ok(Err(_)) | Err(_) => continue, | |
| }; | |
| let request = String::from_utf8_lossy(&buf); | |
| if !is_callback_request(&request, callback_path) { | |
| let _ = stream | |
| .write_all(b"HTTP/1.1 204 No Content\r\nConnection: close\r\n\r\n") | |
| .await; | |
| continue; | |
| } | |
| let html = "<html><body>Login successful. You can close this tab.</body></html>"; | |
| let response = format!( | |
| "HTTP/1.1 200 OK\r\nContent-Type: text/html\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{}", | |
| html.len(), | |
| html | |
| ); | |
| let _ = stream.write_all(response.as_bytes()).await; | |
| return parse_callback(&request); | |
| } | |
| } | |
| async fn read_headers(stream: &mut tokio::net::TcpStream) -> Result<Vec<u8>, Error> { | |
| let mut buf = Vec::with_capacity(4096); | |
| let mut chunk = [0u8; 512]; | |
| loop { | |
| let n = stream.read(&mut chunk).await?; | |
| if n == 0 { | |
| break; | |
| } | |
| if buf.len() + n >= 16384 { | |
| return Err(Error::Callback("request too large".into())); | |
| } | |
| buf.extend_from_slice(&chunk[..n]); | |
| if buf.windows(4).any(|w| w == b"\r\n\r\n") { | |
| break; | |
| } | |
| } | |
| Ok(buf) | |
| } |
🤖 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 `@vendor/motosan-ai-oauth/src/server.rs` around lines 37 - 82, Update
wait_for_callback to apply a per-connection timeout while reading and handling
each accepted stream, so stalled clients cannot block subsequent callbacks.
Isolate connection-specific failures, including read_headers errors such as
“request too large,” by discarding that connection and continuing the accept
loop instead of propagating the error and aborting the login flow.
1 participant
Summary
Why
The release graph carried unused crates, duplicate HTTP clients, and unintended TLS providers. In particular, the OAuth dependency enabled reqwest's default native-TLS backend even though non-Windows builds are intended to use rustls.
The vendored OAuth source is minimized to the Codex provider used by OpenHuman; unused Gemini/Anthropic provider sources are excluded.
Validation
Related follow-ups: #5197, #5198, #5199.
Summary by CodeRabbit
New Features
Bug Fixes
Quality Improvements