fix(llm): image detail field + /v1 base URL normalization by zmanian · Pull Request #2380 · nearai/ironclaw

zmanian · 2026-04-12T22:58:51Z

Summary

Image vision fails with OpenAI-compatible providers: missing 'detail' field on image_url #2378: Set detail: Some("auto") on ImageUrl construction in src/agent/attachments.rs so providers that require the detail field (e.g. MiniMax) no longer reject vision requests. Previously detail: None was skipped by skip_serializing_if, causing failures on strict providers.
openai_compatible: LLM_BASE_URL requires /v1 suffix after rig-core migration #1934: Normalize OpenAI-compatible base URLs by appending /v1 when missing in src/llm/mod.rs. rig-core's openai::Client does not auto-append /v1/, so local model servers (MLX, vLLM, llama.cpp) using bare URLs like http://localhost:8080 got 404s.

Test plan

image_url_includes_detail_auto -- verifies ImageUrl detail field is set to "auto"
test_normalize_openai_base_url_appends_v1_when_missing -- bare URLs get /v1 appended
test_normalize_openai_base_url_leaves_v1_alone -- URLs already containing /v1 are unchanged
test_normalize_openai_base_url_preserves_subpaths -- non-/v1 subpaths get /v1 appended
cargo clippy --all --all-features -- zero warnings
cargo test -- all unit tests pass (pre-existing e2e trace failures unrelated)

Closes #2378, closes #1934

Generated with Claude Code

gemini-code-assist

Code Review

This pull request introduces Ethereum wallet integration using WalletConnect v2, including a new asynchronous callback registry for handling long-running tool operations. Key additions include the wallet_pair and wallet_transact tools, a background event listener, and configuration for Ethereum RPC and WalletConnect. However, several critical issues were identified: the wallet_transact tool fails to actually initiate the transaction request, a potential deadlock exists in the session's event polling due to holding a mutex across an await point, and the git dependency for walletconnect-client should be pinned to a specific revision for build reproducibility. Additionally, the background listener contains unused fields intended for result resolution, and the session status is not correctly updated upon disconnection.

gemini-code-assist · 2026-04-12T23:01:26Z

src/tools/builtin/ethereum/transact.rs

+    async fn execute(
+        &self,
+        params: serde_json::Value,
+        ctx: &JobContext,
+    ) -> Result<ToolOutput, ToolError> {
+        let start = std::time::Instant::now();
+
+        // Must be paired first.
+        if !self.session.is_paired().await {
+            return Err(EthereumError::NotPaired.into());
+        }
+
+        // Extract and validate parameters.
+        let to = require_str(&params, "to")?;
+        if !is_valid_eth_address(to) {
+            return Err(EthereumError::InvalidAddress {
+                address: to.to_string(),
+            }
+            .into());
+        }
+
+        let value = require_str(&params, "value")?;
+        let data = params.get("data").and_then(|v| v.as_str());
+        let chain_id = params.get("chain_id").and_then(|v| v.as_u64());
+
+        // Generate a correlation ID for the async callback.
+        let correlation_id = uuid::Uuid::new_v4().to_string();
+
+        // Extract channel/thread from context metadata for callback routing.
+        let channel = ctx
+            .metadata
+            .get("source_channel")
+            .and_then(|v| v.as_str())
+            .unwrap_or("unknown")
+            .to_string();
+        let thread_id = ctx
+            .metadata
+            .get("thread_id")
+            .and_then(|v| v.as_str())
+            .map(|s| s.to_string());
+
+        // Register callback so the result can be routed back.
+        self.callback_registry
+            .register(
+                correlation_id.clone(),
+                CallbackMetadata {
+                    tool_name: "wallet_transact".to_string(),
+                    user_id: ctx.user_id.clone(),
+                    thread_id,
+                    channel,
+                },
+            )
+            .await;
+
+        let mut result = json!({
+            "status": "pending",
+            "correlation_id": correlation_id,
+            "to": to,
+            "value": value,
+            "message": "Transaction submitted to wallet for approval. \
+                        The wallet owner must confirm on their device."
+        });
+
+        if data.is_some() {
+            result["data_present"] = json!(true);
+        }
+        if let Some(cid) = chain_id {
+            result["chain_id"] = json!(cid);
+        }
+
+        Ok(ToolOutput::success(result, start.elapsed()))
+    }


The execute method in WalletTransactTool registers a callback but fails to actually initiate the Ethereum transaction. It should call self.session.request("eth_sendTransaction", ...) to trigger the signature request. Additionally, when an operation is paused for user interaction (like this transaction approval), ensure that any accumulated metrics are propagated and reported before waiting. Note that if SubmissionResult::AuthPending is returned, it does not need to carry instructions.

References

When an operation is paused for user interaction (e.g., approval), ensure that any accumulated metrics up to that point are propagated and reported before waiting.

SubmissionResult::AuthPending does not need to carry instructions, as persistence happens before it is returned. The consumer only needs to know the pending state.

gemini-code-assist · 2026-04-12T23:01:26Z

src/tools/builtin/ethereum/session.rs

+        let mut client = self.wc_client.lock().await;
+        if let Some(ref wc) = *client {
+            match wc.next().await {
+                Ok(event) => event,
+                Err(e) => {
+                    tracing::warn!("WalletConnect event error: {e}");
+                    // On disconnect error, drop the client
+                    if matches!(e, WalletConnectError::Disconnected) {
+                        *client = None;
+                    }
+                    None
+                }
+            }
+        } else {
+            None
+        }
+    }


The poll_event method holds the wc_client Mutex lock while awaiting wc.next(). Since next() is an asynchronous call that waits for events from the WalletConnect relay, this blocks any other concurrent operations on the session. For example, the request method will be unable to acquire the lock to send transactions while the background listener is polling. This effectively deadlocks the session's ability to send requests while listening for events.

gemini-code-assist · 2026-04-12T23:01:26Z

Cargo.toml

 hex = "0.4.3"

+# WalletConnect v2 client for Ethereum transaction approval
+walletconnect-client = { git = "https://github.com/zmanian/walletconnect-client.git", branch = "main", default-features = false, features = ["native"] }


The git dependency walletconnect-client should be pinned to a specific commit hash (rev) instead of a branch to ensure reproducible builds and prevent unexpected changes from upstream. This follows the project's coding standards for external git dependencies.

Suggested change

walletconnect-client = { git = "https://github.com/zmanian/walletconnect-client.git", branch = "main", default-features = false, features = ["native"] }

walletconnect-client = { git = "https://github.com/zmanian/walletconnect-client.git", rev = "b7ba7c0c6ebe4e996ba3a5a09158dabb81df5733", default-features = false, features = ["native"] }

References

Pin git dependencies to a specific commit hash (rev) for reproducible builds.

gemini-code-assist · 2026-04-12T23:01:26Z

src/tools/builtin/ethereum/listener.rs

+    #[allow(dead_code)]
+    callback_registry: Arc<ToolCallbackRegistry>,
+    #[allow(dead_code)]
+    inject_tx: mpsc::Sender<IncomingMessage>,
+}


The callback_registry and inject_tx fields are marked with #[allow(dead_code)] and are not used in the handle_event logic. These fields are essential for resolving pending transaction callbacks when responses are received from the wallet. Their current state suggests that the asynchronous response handling logic is incomplete.

gemini-code-assist · 2026-04-12T23:01:26Z

src/tools/builtin/ethereum/session.rs

+                    if matches!(e, WalletConnectError::Disconnected) {
+                        *client = None;
+                    }
+                    None


When a WalletConnectError::Disconnected occurs during event polling, the internal wc_client is cleared, but the session's status RwLock is not updated. This results in a stale Paired status even though the connection is dead. The status should be explicitly set to SessionStatus::Disconnected to ensure consistency across the application.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7e31f288de

ℹ️ 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".

chatgpt-codex-connector · 2026-04-12T23:04:03Z

src/tools/builtin/ethereum/transact.rs

+        self.callback_registry
+            .register(
+                correlation_id.clone(),
+                CallbackMetadata {
+                    tool_name: "wallet_transact".to_string(),


Submit WalletConnect RPC before reporting pending transaction

In wallet_transact, the tool registers a callback and immediately returns a pending success payload, but it never actually calls WalletConnectSession::request (or any async worker) to send eth_sendTransaction. In the ETHEREUM_ENABLED flow this means the wallet never receives an approval prompt, so users get a "submitted" response for a transaction that was never sent.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-04-12T23:04:03Z

src/llm/mod.rs

+    let trimmed = url.trim_end_matches('/');
+    if trimmed.ends_with("/v1") {
+        trimmed.to_string()
+    } else {
+        format!("{trimmed}/v1")


Avoid forcing /v1 onto OpenAI-compatible base paths

This normalization unconditionally appends /v1 unless the URL already ends with /v1, which rewrites valid configured paths like Gemini’s OpenAI-compatible base (.../v1beta/openai) to .../v1beta/openai/v1. For ProviderProtocol::OpenAiCompletions backends that intentionally use non-/v1 base paths, requests will be routed to the wrong endpoint and fail.

Useful? React with 👍 / 👎.

…1934) Set detail: "auto" on ImageUrl construction so providers requiring the field (e.g. MiniMax) no longer reject vision requests. Normalize OpenAI-compatible base URLs by appending /v1 when missing, fixing 404s for local model servers (MLX, vLLM, llama.cpp) using bare URLs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

serrrfirat · 2026-04-13T07:53:48Z

src/llm/mod.rs

+/// so local model servers (MLX, vLLM, llama.cpp) using bare URLs like
+/// `http://localhost:8080` get 404s. This mirrors the old `NearAiChatProvider::api_url()`
+/// behavior.
+fn normalize_openai_base_url(url: &str) -> String {


Critical — Breaks built-in providers with non-/v1 paths

normalize_openai_base_url blindly appends /v1 to any URL not ending in /v1. This breaks at least two built-in providers from providers.json:

Zai: https://api.z.ai/api/paas/v4 → https://api.z.ai/api/paas/v4/v1 (404)

Gemini: https://generativelanguage.googleapis.com/v1beta/openai → .../v1beta/openai/v1 (404)

Any user with a custom LLM_BASE_URL containing a non-/v1 version path is also broken.

Suggested fix: Only append /v1 when the URL has no path component at all (just scheme + host + optional port, like http://localhost:8080). Parse the URL and check if the path is / or empty:

fn normalize_openai_base_url(url: &str) -> String { let trimmed = url.trim_end_matches('/'); if trimmed.ends_with("/v1") { return trimmed.to_string(); } match url::Url::parse(trimmed) { Ok(parsed) if parsed.path().is_empty() || parsed.path() == "/" => { format!("{trimmed}/v1") } _ => trimmed.to_string(), } }

serrrfirat · 2026-04-13T07:53:53Z

src/llm/mod.rs

+/// behavior.
+fn normalize_openai_base_url(url: &str) -> String {
+    let trimmed = url.trim_end_matches('/');
+    if trimmed.ends_with("/v1") {


Medium — Inconsistent scope: Ollama path not normalized

The normalization only applies to OpenAiCompletions protocol, not Ollama. create_ollama_from_registry also uses bare base URLs with rig-core's builder. If the intent is to fix bare local URLs (MLX, vLLM, llama.cpp), those can appear under either protocol.

Verify whether Ollama's client handles /v1 differently (it uses /api/chat not /v1/chat/completions), and if so, document the asymmetry.

serrrfirat · 2026-04-13T07:53:56Z

src/llm/mod.rs

+/// behavior.
+fn normalize_openai_base_url(url: &str) -> String {
+    let trimmed = url.trim_end_matches('/');
+    if trimmed.ends_with("/v1") {


Medium — Case-sensitive check

ends_with("/v1") is case-sensitive. A user configuring http://localhost:8080/V1 would get double-suffixed to http://localhost:8080/V1/v1.

Suggested fix: Use trimmed.to_ascii_lowercase().ends_with("/v1") for the check.

serrrfirat · 2026-04-13T07:54:00Z

src/llm/mod.rs

+        assert_eq!(
+            normalize_openai_base_url("https://api.openai.com/v1"),
+            "https://api.openai.com/v1"
+        );


Medium — Missing test cases for real provider URLs

No test covers URLs with non-/v1 versioned subpaths (e.g., /v4, /v1beta/openai). The test_normalize_openai_base_url_preserves_subpaths test uses /custom which incorrectly gets /v1 appended. Adding tests for real provider URLs from providers.json would immediately reveal finding #1.

Suggested tests:

https://api.z.ai/api/paas/v4 → should stay unchanged

https://generativelanguage.googleapis.com/v1beta/openai → should stay unchanged

http://localhost:8080 → should become http://localhost:8080/v1

serrrfirat

Paranoid Architect Review — REQUEST CHANGES

1 Critical, 3 Medium findings.

The image detail fix is correct and safe. However, the /v1 base URL normalization has a critical bug: it breaks at least 2 built-in providers (Zai, Gemini) and any user with a custom non-/v1 versioned base URL.

Ship-blocker: The normalization must be scoped to bare host-only URLs (no path). The current ends_with("/v1") check is too narrow.

Address review feedback: - Only append /v1 when the URL has no path component (bare scheme://host[:port]). URLs with existing paths like Zai's /api/paas/v4 or Gemini's /v1beta/openai are now left unchanged. - Use case-insensitive check for /v1 suffix to prevent double-suffixing URLs like http://localhost:8080/V1. - Document why Ollama is intentionally excluded from normalization (uses /api/chat, not /v1/chat/completions). - Add test cases for real provider URLs from providers.json (Zai, Gemini) and case-insensitive /V1. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

gimli v0.33.1 was yanked on crates.io, causing cargo-deny to fail. Downgrade to v0.33.0 which is the latest non-yanked release compatible with wasmtime 43. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

serrrfirat · 2026-04-13T12:25:54Z

Addressed all review findings in two commits:

Commit 1 — fix(llm): scope /v1 normalization to bare host-only URLs

Finding	Fix
Critical: breaks Zai/Gemini provider URLs	Now uses `url::Url::parse()` — only appends `/v1` when the URL has no path (bare `scheme://host[:port]`). URLs with existing paths like `/api/paas/v4` or `/v1beta/openai` are left unchanged.
Medium: case-sensitive `/v1` check	Uses `.to_ascii_lowercase().ends_with("/v1")` to prevent double-suffixing `/V1`.
Medium: Ollama asymmetry undocumented	Added doc comment explaining Ollama uses `/api/chat` (not `/v1/chat/completions`) so normalization is intentionally not applied.
Medium: missing real provider URL tests	Added test cases for Zai (`/api/paas/v4`), Gemini (`/v1beta/openai`), case-insensitive `/V1`, and generic custom subpaths — all assert unchanged.

Commit 2 — chore: update gimli 0.33.1 -> 0.33.0

gimli v0.33.1 was yanked on crates.io, causing the cargo-deny CI job to fail. Downgraded to v0.33.0 (latest non-yanked release compatible with wasmtime 43). This is unrelated to the PR's code changes.

serrrfirat

Paranoid Architect Review — APPROVED

Zero Critical/High/Medium findings. Reviewed all 3 changed files in full, verified all callers and construction sites.

attachments.rs — detail: Some("auto") is correct. Works for providers serializing ContentPart via serde (the MiniMax fix path). Note: rig_adapter.rs still constructs Image { detail: None } independently — pre-existing, not introduced here.

llm/mod.rs — normalize_openai_base_url is well-scoped:

Only appends /v1 for bare host-only URLs (no path)
Case-insensitive /v1 check prevents double-suffixing
Url::parse failure falls through safely (returns unchanged)
Empty string guarded by caller (!config.base_url.is_empty())
Real provider URLs (Zai /v4, Gemini /v1beta/openai) tested and preserved

Cargo.lock — gimli downgrade from yanked 0.33.1 to 0.33.0, trivially safe.

All prior review findings from @serrrfirat addressed. CI green. Good to merge.

gemini-code-assist bot reviewed Apr 12, 2026

View reviewed changes

chatgpt-codex-connector bot reviewed Apr 12, 2026

View reviewed changes

zmanian force-pushed the fix/llm-provider-compat branch from 7e31f28 to b81aa52 Compare April 12, 2026 23:23

github-actions bot added the size: M 50-199 changed lines label Apr 12, 2026

zmanian requested review from ilblackdragon and serrrfirat April 13, 2026 00:30

github-actions bot mentioned this pull request Apr 13, 2026

🦞 OpenClaw 生态日报 2026-04-13 gsscsd/big_model_radar#178

Open

serrrfirat reviewed Apr 13, 2026

View reviewed changes

serrrfirat requested changes Apr 13, 2026

View reviewed changes

serrrfirat and others added 2 commits April 13, 2026 15:20

chore: update gimli 0.33.1 -> 0.33.0 (yanked crate)

232c4d0

gimli v0.33.1 was yanked on crates.io, causing cargo-deny to fail. Downgrade to v0.33.0 which is the latest non-yanked release compatible with wasmtime 43. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

serrrfirat self-requested a review April 13, 2026 12:37

serrrfirat approved these changes Apr 13, 2026

View reviewed changes

serrrfirat merged commit 160a75e into staging Apr 13, 2026
15 checks passed

serrrfirat deleted the fix/llm-provider-compat branch April 13, 2026 12:41

	walletconnect-client = { git = "https://github.com/zmanian/walletconnect-client.git", branch = "main", default-features = false, features = ["native"] }
	walletconnect-client = { git = "https://github.com/zmanian/walletconnect-client.git", rev = "b7ba7c0c6ebe4e996ba3a5a09158dabb81df5733", default-features = false, features = ["native"] }

Conversation

zmanian commented Apr 12, 2026

Summary

Test plan

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

serrrfirat Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

serrrfirat Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

serrrfirat Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

serrrfirat Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

serrrfirat left a comment

Choose a reason for hiding this comment

Paranoid Architect Review — REQUEST CHANGES

Uh oh!

serrrfirat commented Apr 13, 2026

Uh oh!

serrrfirat left a comment

Choose a reason for hiding this comment

Paranoid Architect Review — APPROVED

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants