Skip to content

feat: enhanced runagent-rust sdk with proper standard#96

Merged
sawradip merged 2 commits into
mainfrom
sawra/sdk_rust_fix
Nov 17, 2025
Merged

feat: enhanced runagent-rust sdk with proper standard#96
sawradip merged 2 commits into
mainfrom
sawra/sdk_rust_fix

Conversation

@sawradip
Copy link
Copy Markdown
Contributor

@sawradip sawradip commented Nov 15, 2025

Summary by CodeRabbit

  • Documentation

    • Added publish checklist and expanded README with installation, configuration, usage patterns, troubleshooting, and multiple examples.
  • New Features

    • New config-centric client creation with builder-style options and extra parameters.
    • Synchronous (blocking) client API and multiple async/sync examples.
    • Improved streaming/deserialization handling and architecture validation.
    • Structured execution errors with codes, messages, suggestions, and details.
  • Bug Fixes

    • More consistent error propagation, clearer diagnostics, and guardrails to prevent stream/non-stream misuse.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Nov 15, 2025

Walkthrough

Introduces a config-centric RunAgentClient API (RunAgentClientConfig), adds a synchronous blocking wrapper, refactors architecture/error handling and deserialization, removes file-based config & many legacy constants, and adds docs/examples and a publishing checklist.

Changes

Cohort / File(s) Summary
Docs & Checklists
runagent-rust/PUBLISH.md, runagent-rust/runagent/README.md, sdk_checklist.md
New PUBLISH.md checklist; README rewritten for Rust bindings and config-centric usage; sdk_checklist.md adds architecture contract, guardrails, error taxonomy, and repo hygiene requirements.
Client config & public API
runagent-rust/runagent/src/client/runagent_client.rs, runagent-rust/runagent/src/client/mod.rs, runagent-rust/runagent/src/lib.rs
Adds public RunAgentClientConfig and builder-style setters; replaces old constructor with RunAgentClient::new(config); exposes extra_params, is_local, remote client creation helper, initialize_architecture() orchestration; re-exports config and blocking client.
Blocking (synchronous) wrapper
runagent-rust/runagent/src/blocking.rs, runagent-rust/runagent/examples/*_example.rs
New blocking RunAgentClient wrapping async client + Tokio runtime; blocking run/run_stream collection APIs; added sync & async examples demonstrating direct construction and usage.
Errors & execution model
runagent-rust/runagent/src/types/errors.rs, runagent-rust/runagent/src/client/rest_client.rs
Adds RunAgentError::Execution { code, message, suggestion, details } and constructor; adjusts category/retry logic; refactors get_agent_architecture to extract nested error/data and return structured Execution errors.
Deserialization & streaming
runagent-rust/runagent/src/utils/serializer.rs, runagent-rust/runagent/src/client/socket_client.rs
Adds prepare_for_deserialization and extended deserialize_object to handle string-encoded JSON, legacy shapes, and payload variants; socket client now deserializes stream data, returning structured values or explicit errors.
Constants & config persistence
runagent-rust/runagent/src/constants.rs, runagent-rust/runagent/src/utils/config.rs, runagent-rust/runagent/src/db/service.rs, runagent-rust/runagent/src/types/schema.rs
Removes many legacy constants and local-cache artifacts; introduces runtime-resolved local cache/db path (env / ~/.runagent), DEFAULT_BASE_URL / API prefix / timeout; file-based config persistence removed in favor of env-driven load(); Database path and schema defaults updated to use ~/.runagent runtime path.
Examples & tests
runagent-rust/examples/*, runagent-rust/runagent/examples/*, runagent-rust/runagent/examples/test_deserialize.rs
Adds multiple examples (async, sync, direct construction, omitted fields, deserialize test) illustrating new config API, extra_params usage, and serializer behavior.
Misc docs & packaging
runagent-rust/PUBLISH.md, sdk_checklist.md
Publishing and SDK contract documentation added/updated to reflect new flows and repository requirements.

Sequence Diagram(s)

sequenceDiagram
    participant App as Application
    participant Config as RunAgentClientConfig
    participant Client as RunAgentClient
    participant Rest as RestClient
    participant Socket as SocketClient
    participant API as Remote API

    rect `#f0f8ff`
    Note over App,Client: Config-centric initialization
    App->>Config: Build config (agent_id, entrypoint_tag, local?, base_url?, api_key?, extra_params?)
    App->>Client: RunAgentClient::new(config)
    Client->>Client: resolve env/base_url/api_key/host/port
    alt remote
        Client->>Client: create_remote_clients(base_url_override, api_key_override)
        Client->>Rest: instantiate RestClient
        Client->>Socket: instantiate SocketClient
    else local
        Client->>Client: use local address or DB lookup (if enabled)
    end
    Client->>Client: initialize_architecture()
    Client->>Rest: get_agent_architecture()
    Rest->>API: GET /api/v1/agents/{id}/architecture
    API-->>Rest: envelope { data | error }
    alt envelope.error
        Rest->>Client: RunAgentError::Execution { code, message, suggestion, details }
    else envelope.data
        Rest->>Client: AgentArchitecture
        Client->>Client: validate entrypoint, set internal metadata
    end
    Client-->>App: Ready or Error
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Areas needing careful review:
    • runagent_client.rs: new config flow, resolution of host/port/env fallbacks, extra_params propagation, and initialize_architecture() error paths.
    • rest_client.rs & errors.rs: correct extraction/mapping of nested envelope errors into Execution variant and retry semantics.
    • socket_client.rs & serializer.rs: streaming deserialization logic—ensure parsing, error yields, and backward-compatibility are correct and safe.
    • constants.rs / config.rs / db/service.rs: removal of many constants and change to env/fileless config; verify no remaining references and DB path creation permissions/behavior.
    • Blocking wrapper: runtime initialization, thread-safety, and correct conversion of async streams into blocking results.

Possibly related PRs

Poem

🐇 I hopped in code at break of day,
A Config here, a client made of clay,
Execution errors now sing with a cue,
Streams parse strings and examples grew,
Docs and publish steps tucked in my burrow too! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title is vague and does not clearly convey the specific changes made. 'Enhanced with proper standard' is non-descriptive and does not summarize the main architectural changes to the SDK. Revise the title to be more specific and descriptive of the primary changes, such as: 'feat: add RunAgentClientConfig builder pattern and refactor client initialization' or 'feat: restructure SDK with config-driven initialization and blocking client support'.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 89.47% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch sawra/sdk_rust_fix

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sawradip sawradip changed the title feat: enhanced runagent-rust sdk with porper standard feat: enhanced runagent-rust sdk with proper standard Nov 15, 2025
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (9)
sdk_checklist.md (1)

145-151: Generalize repo hygiene/publish wording beyond npm

The “Repository hygiene” bullet currently says every SDK repo must ship a PUBLISH.md with “npm publish guidance”. For Rust (and other non-JS SDKs) this is cargo publish (or pip, etc.), not npm.

Suggest rewording to something like “language-appropriate publish guidance (e.g. cargo publish, npm publish, pip publish, …)” so this stays accurate across SDKs.

runagent-rust/runagent/src/types/errors.rs (1)

125-138: Small constructor ergonomics + tests for Execution behavior

Two minor improvements you might consider:

  • Loosen the execution signature so code and message can have different source types without extra .to_string()s:
-    pub fn execution<S: Into<String>>(
-        code: S,
-        message: S,
+    pub fn execution<C: Into<String>, M: Into<String>>(
+        code: C,
+        message: M,
         suggestion: Option<String>,
         details: Option<Value>,
     ) -> Self {
  • Add a focused test that:
    • Constructs a RunAgentError::execution("SERVER_ERROR", ...) and asserts category() == "execution" and is_retryable() is true.
    • Constructs an Execution with a non-retryable code (e.g. "ARCHITECTURE_MISSING") and asserts is_retryable() is false.

These are non-blocking but would make the API a bit friendlier and lock in the intended semantics.

Also applies to: 166-172

runagent-rust/runagent/README.md (2)

52-65: Clarify optional dotenvy dependency in the example

The cloud example uses:

dotenvy::from_filename("local.env").ok(); // optional

but dotenvy isn’t mentioned in the preceding install/dependency snippets. To avoid “unresolved crate” surprises for users, either:

  • Add dotenvy to the sample [dependencies] (with a note that it’s optional), or
  • Drop the call from the example and instead mention env-loading as an optional tip in text.

Either approach keeps the example copy‑pasteable.


186-190: Avoid bare docs URL for consistency

The “Need Help?” section uses a bare URL:

Docs: docs/sdk/rust/ (coming soon) or https://docs.rs/runagent

For consistency with the rest of the README (and to silence MD034), consider changing it to something like:

Docs: `docs/sdk/rust/` (coming soon) or [docs.rs/runagent](https://docs.rs/runagent)
runagent-rust/runagent/src/client/runagent_client.rs (5)

24-24: extra_params field is only stored/exposed, not yet used in calls

Right now extra_params is populated on the client and exposed via extra_params(), but it doesn’t influence run/run_stream or any client behavior. If this is just forward‑compat wiring that’s fine; otherwise consider either threading it into the request payloads or omitting it until it’s used to avoid confusion about its effect.

Also applies to: 406-409


30-55: RunAgentClientOptions API looks solid; consider Default/builder ergonomics

The options struct and new ctor are straightforward and make the construction paths clearer. If this grows further, you might want a Default impl and/or a builder to avoid repetitive literal initializations, but that’s purely ergonomics.


74-83: Nice reuse of from_options; double‑check DB port type conversion

Routing RunAgentClient::new through from_options for both DB‑backed and remote setups looks good and keeps initialization centralized.

One small concern: agent_info.port as u16 will silently truncate if agent_info.port isn’t already constrained to the 0..=65535 range. If the DB schema uses a wider integer type, consider either storing it as u16 directly or validating with u16::try_from(agent_info.port) and returning a clear RunAgentError::validation on failure.

Also applies to: 103-112


232-242: Logging available entrypoints is helpful; consider surfacing in error text

The new available list and tracing::error! log are very useful for debugging missing entrypoint tags. You might also consider including the available tags in the RunAgentError::validation message itself so users without log access get the same hint, e.g., appending Available entrypoints: [...] to the error string.


417-438: create_remote_clients centralizes remote wiring; align ws URL fallback and constants

Centralizing remote client creation here is good. A couple of polish points:

  • In runagent-rust/runagent/src/client/socket_client.rs::default, the fallback for base URLs without http(s):// is ws://{base_url}, whereas here the fallback is wss://{base_url}. For consistency (and to avoid surprising behavior if someone ever passes a bare host), consider matching the default implementation’s behavior:
-            } else {
-                format!("wss://{}", base_url)
-            };
+            } else {
+                format!("ws://{}", base_url)
+            };
  • The "/api/v1" literal is now duplicated in several places (here and in the local path). If this prefix is expected to change over time, you might want a single constant to avoid drift.

These are quality‑of‑life tweaks; the current behavior is otherwise sound given that valid base_url values are expected to be full http(s) URLs.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 242a84a and 2bb2088.

📒 Files selected for processing (6)
  • runagent-rust/PUBLISH.md (1 hunks)
  • runagent-rust/runagent/README.md (1 hunks)
  • runagent-rust/runagent/src/client/rest_client.rs (1 hunks)
  • runagent-rust/runagent/src/client/runagent_client.rs (7 hunks)
  • runagent-rust/runagent/src/types/errors.rs (5 hunks)
  • sdk_checklist.md (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
runagent-rust/runagent/src/types/errors.rs (3)
runagent-rust/runagent/src/client/rest_client.rs (1)
  • serde_json (71-71)
runagent-rust/runagent/src/client/runagent_client.rs (3)
  • serde_json (289-289)
  • serde_json (308-308)
  • serde_json (326-326)
examples/lead-agent/backend-rust/src/main.rs (3)
  • serde_json (158-158)
  • serde_json (165-165)
  • serde_json (172-172)
runagent-rust/runagent/src/client/runagent_client.rs (4)
runagent-rust/runagent/src/client/rest_client.rs (2)
  • new (21-41)
  • default (44-51)
runagent-rust/runagent/src/client/socket_client.rs (2)
  • new (25-38)
  • default (41-55)
runagent-rust/runagent/src/db/service.rs (1)
  • new (26-46)
runagent-rust/runagent/src/utils/config.rs (3)
  • base_url (198-200)
  • api_key (193-195)
  • default (27-37)
runagent-rust/runagent/src/client/rest_client.rs (2)
runagent-rust/runagent/src/types/responses.rs (1)
  • success (17-25)
runagent-rust/runagent/src/types/errors.rs (1)
  • execution (126-138)
🪛 LanguageTool
runagent-rust/PUBLISH.md

[style] ~15-~15: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...t/Cargo.toml(workspace) if needed. 3. Update the version badge/examples inrunagent...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

🪛 markdownlint-cli2 (0.18.1)
runagent-rust/runagent/README.md

188-188: Bare URL used

(MD034, no-bare-urls)

🔇 Additional comments (6)
runagent-rust/PUBLISH.md (1)

1-49: Publishing checklist looks solid

The flow (version bump → fmt/clippy/test → cargo package audit → cargo publish → tagging/docs) is clear and matches typical Rust crate publishing practice. No changes needed from my side.

runagent-rust/runagent/src/types/errors.rs (1)

42-50: Structured Execution variant is well-aligned with the new error model

The Execution { code, message, suggestion, details } variant plus the execution() helper and category()/is_retryable() updates give you the structured surface the docs describe. The retryability check on "CONNECTION_ERROR"/"SERVER_ERROR" codes is a nice way to reuse existing retry logic.

Also applies to: 125-138, 158-172

runagent-rust/runagent/src/client/rest_client.rs (1)

220-279: All call sites verified; changes are correct

Verification confirms that get_agent_architecture() changes align perfectly with how the result is consumed:

  • initialize_architecture() stores the result in agent_architecture: Option<Value>
  • validate_entrypoint() immediately calls .get("entrypoints") on it, expecting the data object directly (not the full response envelope)
  • This matches the API contract: { success: bool, data: { entrypoints: [...], ... } } → extracting and returning data

All internal call sites are correctly aligned with the new behavior. No issues found.

runagent-rust/runagent/src/client/runagent_client.rs (3)

123-133: with_address delegation looks good

Delegating with_address through RunAgentClientOptions/from_options keeps the local/remote logic in one place and lets the validation in from_options handle missing host/port nicely. No issues here.


135-152: with_remote_config is a useful explicit remote entrypoint

The explicit with_remote_config helper is clear and keeps remote configuration explicit (local: false, base_url, api_key). Behavior is consistent with the new options‑based flow.


367-371: Symmetric guard for streaming entrypoints; confirm backwards‑compat

The new guard that rejects run_stream_with_args when entrypoint_tag doesn’t end with _stream mirrors the existing check in run and enforces the naming convention more strictly.

This is a behavior change: previously you could call run_stream* on non‑*_stream tags. Please confirm no existing agents rely on streaming semantics without that suffix, or consider allowing an override based on architecture metadata instead of solely naming.

Comment on lines +154 to +165
/// Build a RunAgent client from detailed options
pub async fn from_options(options: RunAgentClientOptions<'_>) -> RunAgentResult<Self> {
let serializer = CoreSerializer::new(10.0)?;
#[cfg(feature = "db")]
let db_service = if local {
let db_service = if options.local {
Some(DatabaseService::new(None).await?)
} else {
None
};

let (rest_client, socket_client) = if local {
let (agent_host, agent_port) = if let (Some(h), Some(p)) = (host, port) {
tracing::info!("🔌 Using explicit address: {}:{}", h, p);
(h.to_string(), p)
} else {
#[cfg(feature = "db")]
{
if let Some(ref db) = db_service {
let agent_info = db.get_agent(agent_id).await?
.ok_or_else(|| RunAgentError::validation(format!("Agent {} not found in local DB", agent_id)))?;

tracing::info!("🔍 Auto-resolved address for agent {}: {}:{}", agent_id, agent_info.host, agent_info.port);
(agent_info.host, agent_info.port as u16)
} else {
return Err(RunAgentError::config("Database feature not enabled but required for local agent lookup"));
}
}
#[cfg(not(feature = "db"))]
{
return Err(RunAgentError::config("Database feature not enabled but required for local agent lookup"));
}
};
#[cfg(not(feature = "db"))]
let db_service: Option<DatabaseService> = None;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Fix cfg(non‑db) path and minor construction/initialization refinements

  1. Non‑db build will not compile due to DatabaseService reference

In from_options:

#[cfg(not(feature = "db"))]
let db_service: Option<DatabaseService> = None;

When the db feature is disabled, DatabaseService isn’t imported (and may not exist at all), so this line will fail to compile. You also don’t need a db_service local in the non‑db configuration because the struct field is itself behind #[cfg(feature = "db")].

You can safely drop the non‑db binding:

-        #[cfg(not(feature = "db"))]
-        let db_service: Option<DatabaseService> = None;

The #[cfg(feature = "db")] db_service, field initialization below already guards usage appropriately.

  1. Optional: reduce cloning and destructure options

You currently clone options.api_key and options.extra_params:

  • Self::create_remote_clients(options.base_url, options.api_key.clone())?
  • extra_params: options.extra_params.clone(),

Since options is taken by value and not used after construction, you could destructure it to avoid redundant clones:

let RunAgentClientOptions {
    agent_id,
    entrypoint_tag,
    local,
    host,
    port,
    base_url,
    api_key,
    extra_params,
} = options;

and then use owned values directly. This is a nice‑to‑have, not required.

  1. Architecture initialization behavior change

client.initialize_architecture().await?; now forces an architecture fetch and entrypoint validation during construction. That’s great for early failure and better error messages, but it does add a network round‑trip to all client creations. If you have scenarios where lazy loading is preferable (e.g., lightweight health checks or offline tests), you might eventually want an opt‑out constructor or a skip_arch_validation flag in RunAgentClientOptions.

Also applies to: 166-189, 191-200, 205-206, 210-218

🤖 Prompt for AI Agents
In runagent-rust/runagent/src/client/runagent_client.rs around lines 154 to 165,
remove the #[cfg(not(feature = "db"))] let db_service: Option<DatabaseService> =
None binding because DatabaseService may not exist when the db feature is
disabled and the struct field is already feature-gated; instead rely on the
#[cfg(feature = "db")] db_service initialization used in the struct initializer.
While here, destructure the owned RunAgentClientOptions passed into from_options
to take ownership of fields (agent_id, entrypoint_tag, local, host, port,
base_url, api_key, extra_params) and use those owned values to avoid unnecessary
clones. Finally, be aware that client.initialize_architecture().await? forces a
network round-trip during construction—if you need lazy initialization, add a
skip_arch_validation flag to RunAgentClientOptions and conditionally call
initialize_architecture.

- Introduced RunAgentClientConfig struct for improved configuration management.
- Updated client initialization to support builder pattern for optional parameters.
- Enhanced documentation with examples for creating clients using both direct construction and builder pattern.
- Improved error handling and logging for local agent configuration.
- Added support for enabling/disabling database registry lookup in client configuration.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

♻️ Duplicate comments (1)
runagent-rust/runagent/src/client/runagent_client.rs (1)

220-224: Fix compilation error when db feature is disabled.

The non-db cfg block at lines 223-224 references DatabaseService, which is not available when the db feature is disabled. The type is only imported and defined behind #[cfg(feature = "db")], causing a compilation failure in the non-db case. Since the db_service field itself doesn't exist in the struct when the feature is disabled, no binding is needed in that case.

Remove the non-db block:

         let serializer = CoreSerializer::new(10.0)?;
         #[cfg(feature = "db")]
         let db_service: Option<DatabaseService> = None;
-        #[cfg(not(feature = "db"))]
-        let db_service: Option<DatabaseService> = None;
🧹 Nitpick comments (11)
runagent-rust/runagent/examples/test_deserialize.rs (1)

4-16: Example usage of CoreSerializer looks correct

The example matches the { type, payload } contract and demonstrates the nested string payload case well. If you want to make it a bit more idiomatic as a reference sample, you could switch main to return RunAgentResult<()> and use ? instead of unwrap(), but it's not required for a simple example.

runagent-rust/runagent/src/client/socket_client.rs (1)

103-151: Good reuse of shared deserialization; consider preserving the original error

Reusing CoreSerializer for "data" messages (prepare_for_deserializationdeserialize_object) is a solid improvement and keeps WebSocket behavior aligned with the HTTP path.

In the error branch:

match serializer.deserialize_object(prepared) {
    Ok(deserialized) => yield Ok(deserialized),
    Err(e) => {
        yield Err(RunAgentError::server(format!("Deserialization error: {}", e)));
        break;
    }
}

wrapping e into a new Server error loses any execution code/category the serializer might set. If callers care about those distinctions, you could instead yield Err(e) directly (and still break), or wrap only when you explicitly want to reclassify.

Non-blocking, but would keep the error taxonomy more informative end-to-end.

runagent-rust/runagent/src/constants.rs (1)

11-21: Align DEFAULT_TIMEOUT_SECONDS with actual timeouts or clarify intent

DEFAULT_TIMEOUT_SECONDS is documented as 5 minutes and set to 300, but the REST client currently:

  • Sets the HTTP client timeout to 600 seconds.
  • Sends "timeout_seconds": 600 in run_agent payloads.

If DEFAULT_TIMEOUT_SECONDS is meant to be the single source of truth for agent execution timeouts, consider:

  • Updating it to 600 and using it in the REST/WebSocket clients, or
  • Keeping it at 300 but documenting that it’s a separate client-level default distinct from backend execution timeouts.

Right now the divergence is a bit confusing and could lead to inconsistent behavior when new code starts consuming this constant.

runagent-rust/runagent/src/types/schema.rs (1)

308-317: Centralizing the client base URL default is a nice improvement

Using crate::constants::DEFAULT_BASE_URL.to_string() in ClientConfig::default is a good way to keep the SDK’s default endpoint in one place. Given this, it may be worth documenting (or aligning) how this default interacts with Config::load() and any environment overrides so that users have a clear precedence order (env/config vs. built‑in default).

Also note that the timeout: 30 here is a per-request client timeout, while DEFAULT_TIMEOUT_SECONDS is 300; if both are intended to exist, a brief doc comment clarifying the difference (client request vs. agent execution) would reduce confusion later.

runagent-rust/examples/async_example.rs (1)

1-27: Consider deduplicating or clearly distinguishing the two async examples

This file appears to duplicate runagent-rust/runagent/examples/async_example.rs. If both are needed (workspace vs crate), consider:

  • Adding a brief comment explaining why both exist, and/or
  • Ensuring there’s a single source of truth (e.g., keep one minimal and link to it from docs) to reduce the chance of future divergence.
runagent-rust/examples/sync_example.rs (1)

1-27: Sync example duplicates crate-level example; consider avoiding divergence

This sync example closely mirrors runagent-rust/runagent/examples/sync_example.rs. If you keep both, a short comment about their distinct purposes and periodically checking they stay in sync would help avoid subtle drift.

runagent-rust/runagent/src/db/service.rs (1)

14-25: Consider warning when falling back to current directory.

The fallback to . when home_dir() fails is a reasonable default, but it could lead to subtle issues if the database file is created in different directories depending on the environment. Consider logging a warning when the fallback is used so developers can diagnose path-related issues more easily.

Apply this diff to add a warning:

 static LOCAL_CACHE_DIRECTORY: Lazy<PathBuf> = Lazy::new(|| {
     // Check environment variable first
     if let Ok(env_path) = std::env::var(ENV_LOCAL_CACHE_DIRECTORY) {
         return PathBuf::from(env_path);
     }
     
     // Default to ~/.runagent
-    dirs::home_dir()
-        .unwrap_or_else(|| PathBuf::from("."))
-        .join(".runagent")
+    match dirs::home_dir() {
+        Some(home) => home.join(".runagent"),
+        None => {
+            tracing::warn!("Could not determine home directory, using current directory for cache");
+            PathBuf::from("./.runagent")
+        }
+    }
 });
runagent-rust/runagent/src/utils/serializer.rs (1)

50-99: Consider simplifying the nested branches or adding examples.

The deserialize_object method now handles four different payload formats, which is good for backward compatibility. However, the nested if-let chains make the logic harder to follow. Consider either:

  1. Extracting each format case into a named helper method
  2. Adding code comments with example JSON for each branch

This would improve maintainability when debugging deserialization issues.

runagent-rust/runagent/src/utils/config.rs (1)

63-97: Consider deprecating the unused _save parameter.

The _save parameter is now ignored but kept for API compatibility. Consider adding a #[deprecated] attribute or a doc comment noting that this parameter is unused, to guide users toward the new configuration model.

 /// Setup and validate configuration
 /// Note: Configuration is stored in SQLite database, not in files
+/// 
+/// The `_save` parameter is ignored (kept for API compatibility).
 pub fn setup(
     api_key: Option<String>,
     base_url: Option<String>,
     _save: bool,
 ) -> RunAgentResult<Self> {
runagent-rust/runagent/src/blocking.rs (1)

44-55: Consider documenting the runtime overhead.

Each RunAgentClient instance creates its own Tokio runtime, which has non-trivial overhead (thread spawning, scheduler initialization). For applications creating many clients, this could be inefficient. Consider adding a doc comment warning about this, or providing an alternative constructor that accepts a shared runtime handle.

Add documentation:

 /// Create a new blocking RunAgent client
 ///
 /// This will create a Tokio runtime internally and block until the client is initialized.
+/// 
+/// Note: Each client creates its own runtime. For applications creating many clients,
+/// consider using the async client with a shared runtime for better resource usage.
 pub fn new(config: RunAgentClientConfig) -> RunAgentResult<Self> {
runagent-rust/runagent/src/client/runagent_client.rs (1)

270-311: Excellent fail-fast architecture initialization with helpful error messages.

The initialize_architecture method during construction ensures early validation, and the enhanced error logging at lines 292-302 that lists available entrypoints will be very helpful for debugging. This is a significant improvement in developer experience.

One consideration: this adds a network round-trip to every client construction. For scenarios like offline tests or lightweight health checks, you might eventually want an opt-out flag (e.g., skip_architecture_validation in config), but this is not urgent.

Would you like me to generate a verification script to check that the entrypoint validation correctly lists available options when an invalid entrypoint is provided?

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2bb2088 and 2cd3225.

📒 Files selected for processing (20)
  • runagent-rust/examples/async_example.rs (1 hunks)
  • runagent-rust/examples/direct_construction.rs (1 hunks)
  • runagent-rust/examples/sync_example.rs (1 hunks)
  • runagent-rust/runagent/README.md (1 hunks)
  • runagent-rust/runagent/examples/async_example.rs (1 hunks)
  • runagent-rust/runagent/examples/direct_construction.rs (1 hunks)
  • runagent-rust/runagent/examples/direct_construction_omitted.rs (1 hunks)
  • runagent-rust/runagent/examples/sync_example.rs (1 hunks)
  • runagent-rust/runagent/examples/test_deserialize.rs (1 hunks)
  • runagent-rust/runagent/src/blocking.rs (1 hunks)
  • runagent-rust/runagent/src/client/mod.rs (1 hunks)
  • runagent-rust/runagent/src/client/rest_client.rs (1 hunks)
  • runagent-rust/runagent/src/client/runagent_client.rs (5 hunks)
  • runagent-rust/runagent/src/client/socket_client.rs (2 hunks)
  • runagent-rust/runagent/src/constants.rs (1 hunks)
  • runagent-rust/runagent/src/db/service.rs (1 hunks)
  • runagent-rust/runagent/src/lib.rs (2 hunks)
  • runagent-rust/runagent/src/types/schema.rs (2 hunks)
  • runagent-rust/runagent/src/utils/config.rs (4 hunks)
  • runagent-rust/runagent/src/utils/serializer.rs (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • runagent-rust/runagent/examples/direct_construction.rs
🧰 Additional context used
🧬 Code graph analysis (12)
runagent-rust/runagent/examples/test_deserialize.rs (1)
runagent-rust/runagent/src/utils/serializer.rs (1)
  • new (15-19)
runagent-rust/runagent/examples/async_example.rs (3)
runagent-rust/examples/direct_construction.rs (1)
  • main (9-27)
runagent-rust/examples/sync_example.rs (1)
  • main (9-26)
runagent-rust/runagent/src/client/runagent_client.rs (4)
  • agent_id (450-452)
  • entrypoint_tag (455-457)
  • new (86-98)
  • new (176-268)
runagent-rust/runagent/src/client/socket_client.rs (2)
runagent-rust/runagent/src/utils/serializer.rs (2)
  • prepare_for_deserialization (38-48)
  • deserialize_object (58-100)
runagent-rust/runagent/src/types/errors.rs (1)
  • server (91-95)
runagent-rust/runagent/src/client/rest_client.rs (2)
runagent-rust/runagent/src/types/responses.rs (1)
  • success (17-25)
runagent-rust/runagent/src/types/errors.rs (1)
  • execution (126-138)
runagent-rust/examples/sync_example.rs (3)
runagent-rust/examples/async_example.rs (1)
  • main (9-26)
runagent-rust/runagent/src/blocking.rs (3)
  • agent_id (126-128)
  • entrypoint_tag (131-133)
  • new (48-55)
runagent-rust/runagent/src/client/runagent_client.rs (4)
  • agent_id (450-452)
  • entrypoint_tag (455-457)
  • new (86-98)
  • new (176-268)
runagent-rust/runagent/src/blocking.rs (4)
runagent-rust/runagent/src/client/rest_client.rs (4)
  • serde_json (71-71)
  • new (21-41)
  • get_agent_architecture (221-279)
  • health_check (282-284)
runagent-rust/runagent/src/client/runagent_client.rs (12)
  • new (86-98)
  • new (176-268)
  • run (314-316)
  • run_with_args (319-404)
  • run_stream (407-412)
  • run_stream_with_args (415-434)
  • get_agent_architecture (437-439)
  • health_check (442-447)
  • agent_id (450-452)
  • entrypoint_tag (455-457)
  • extra_params (460-462)
  • is_local (465-467)
runagent-rust/runagent/src/client/socket_client.rs (2)
  • new (25-38)
  • run_stream (71-181)
runagent-rust/runagent/src/types/errors.rs (1)
  • connection (84-88)
runagent-rust/runagent/examples/direct_construction_omitted.rs (2)
runagent-rust/examples/direct_construction.rs (1)
  • main (9-27)
runagent-rust/runagent/src/client/runagent_client.rs (3)
  • new (86-98)
  • new (176-268)
  • default (104-116)
runagent-rust/examples/async_example.rs (3)
runagent-rust/examples/sync_example.rs (1)
  • main (9-26)
runagent-rust/runagent/src/blocking.rs (3)
  • agent_id (126-128)
  • entrypoint_tag (131-133)
  • new (48-55)
runagent-rust/runagent/src/client/runagent_client.rs (4)
  • agent_id (450-452)
  • entrypoint_tag (455-457)
  • new (86-98)
  • new (176-268)
runagent-rust/runagent/src/utils/config.rs (4)
runagent-rust/runagent/src/types/errors.rs (1)
  • config (119-123)
runagent-rust/runagent/src/client/rest_client.rs (1)
  • default (44-51)
runagent-rust/runagent/src/client/socket_client.rs (1)
  • default (41-55)
runagent-rust/runagent/src/types/schema.rs (4)
  • default (76-81)
  • default (260-268)
  • default (282-295)
  • default (309-317)
runagent-rust/runagent/examples/sync_example.rs (3)
runagent-rust/examples/async_example.rs (1)
  • main (9-26)
runagent-rust/runagent/src/blocking.rs (3)
  • agent_id (126-128)
  • entrypoint_tag (131-133)
  • new (48-55)
runagent-rust/runagent/src/client/runagent_client.rs (4)
  • agent_id (450-452)
  • entrypoint_tag (455-457)
  • new (86-98)
  • new (176-268)
runagent-rust/runagent/src/client/runagent_client.rs (5)
runagent-rust/runagent/src/blocking.rs (5)
  • new (48-55)
  • agent_id (126-128)
  • entrypoint_tag (131-133)
  • extra_params (136-138)
  • is_local (141-143)
runagent-rust/runagent/src/client/rest_client.rs (2)
  • new (21-41)
  • default (44-51)
runagent-rust/runagent/src/client/socket_client.rs (2)
  • new (25-38)
  • default (41-55)
runagent-rust/runagent/src/db/service.rs (1)
  • new (45-65)
runagent-rust/runagent/src/utils/serializer.rs (1)
  • new (15-19)
runagent-rust/examples/direct_construction.rs (1)
runagent-rust/runagent/src/client/runagent_client.rs (2)
  • new (86-98)
  • new (176-268)
🪛 markdownlint-cli2 (0.18.1)
runagent-rust/runagent/README.md

313-313: Bare URL used

(MD034, no-bare-urls)

🔇 Additional comments (11)
runagent-rust/runagent/src/types/schema.rs (1)

283-295: DB path defaults are consistent and directory creation is properly handled—no issues found.

Verification confirms both concerns from the original review are already satisfied:

  1. Directory creation: db/service.rs explicitly calls std::fs::create_dir_all(parent) at lines 52–54 before opening the database pool, ensuring ~/.runagent exists at runtime.

  2. Path consistency: Both schema.rs:283–287 and db/service.rs:21–25 construct the same path (~/.runagent/runagent_local.db). The actual initialization path through DatabaseService::new(None) defaults to LOCAL_CACHE_DIRECTORY.join(DATABASE_FILE_NAME), matching the pattern in the defaults snippet.

The code is correct as-is.

runagent-rust/runagent/examples/async_example.rs (1)

1-26: Async example usage looks correct and idiomatic

Config construction, local setup, and run invocation align with the new RunAgentClientConfig API and are clear for users.

runagent-rust/runagent/examples/sync_example.rs (1)

1-27: Blocking example correctly wraps the async client

The example cleanly demonstrates the blocking RunAgentClient flow with config-based construction and matches the async example behavior.

runagent-rust/runagent/src/client/mod.rs (1)

7-10: Re-exporting RunAgentClientConfig is appropriate

Exporting RunAgentClientConfig next to RunAgentClient from this module matches the new config-centric API and simplifies public usage.

runagent-rust/runagent/src/utils/serializer.rs (1)

33-48: LGTM! Well-documented helper for JSON string handling.

The prepare_for_deserialization method cleanly handles the case where responses arrive as JSON strings, with appropriate fallback to the original value. The documentation is clear about the use case.

runagent-rust/runagent/src/lib.rs (1)

237-265: LGTM! Clean module documentation and exports.

The blocking module documentation is comprehensive with clear examples, and the public API expansion (RunAgentClientConfig, BlockingRunAgentClient) follows Rust conventions. The re-export pattern is consistent with the rest of the codebase.

runagent-rust/runagent/README.md (1)

27-60: LGTM! Clear configuration documentation.

The configuration overview with the comparison table effectively explains the different modes (cloud, local auto-discovery, local explicit). The documentation clearly communicates when each setting applies.

runagent-rust/runagent/src/utils/config.rs (1)

40-60: LGTM! Clean transition to environment-only configuration.

The removal of file-based configuration loading in favor of environment variables simplifies the configuration model. The comments clearly document that configuration is now stored in SQLite, and the code correctly reads from environment variables with proper URL formatting.

runagent-rust/runagent/src/blocking.rs (1)

74-93: LGTM! Stream collection behavior is clearly documented.

The run_stream method correctly documents that it collects the entire stream into a Vec, which loses the streaming benefits. The documentation appropriately directs users to the async client for true streaming. The implementation correctly handles the async stream collection.

runagent-rust/runagent/src/client/runagent_client.rs (2)

31-154: Well-designed configuration API with good defaults.

The RunAgentClientConfig struct and its builder methods provide a clean, flexible API. The combination of direct struct construction (..RunAgentClientConfig::default()) and builder pattern (.with_*()) gives users choice. The documentation examples are comprehensive.

One minor observation: the default() method creates dummy empty strings for required fields. Consider adding a doc comment noting that the dummy values will be overridden when using ..RunAgentClientConfig::default().


470-491: Clean helper for remote client creation.

The create_remote_clients helper centralizes the URL conversion logic (http/https → ws/wss) and client construction. The fallback to default clients when no base_url is provided maintains backward compatibility.

Comment on lines +11 to +21
let client = RunAgentClient::new(runagent::RunAgentClientConfig {
agent_id: "a6977384-6c88-40dc-a629-e6bf077786ae".to_string(),
entrypoint_tag: "minimal".to_string(),
api_key: Some("rau_b4dcebdef6386726b08971a1cc968d8a2b77c5834d30f3f5a43bddf065cd95cb".to_string()),
base_url: Some("http://localhost:8333/".to_string()),
local: None,
host: None,
port: None,
extra_params: None,
enable_registry: None,
}).await?;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Avoid committing what looks like a real API key in the example

The api_key field is populated with a long literal starting with rau_, which looks like a real credential. Even if it’s non-functional, committing secret-like strings is a bad practice and can trigger secret scanners.

Replace it with a placeholder or load from an environment variable. For example:

-        api_key: Some("rau_b4dcebdef6386726b08971a1cc968d8a2b77c5834d30f3f5a43bddf065cd95cb".to_string()),
+        api_key: Some("YOUR_RUNAGENT_API_KEY".to_string()),

(or use std::env::var("RUNAGENT_API_KEY") if you want to show a more realistic pattern).

🤖 Prompt for AI Agents
In runagent-rust/examples/direct_construction.rs around lines 11 to 21, the
example hardcodes a long api_key literal that resembles a real credential;
remove the literal and replace it with a non-secret placeholder or load it from
an environment variable. Update the api_key field to use either None or a call
to std::env::var("RUNAGENT_API_KEY") (handling the Result) or set
Some("YOUR_API_KEY_HERE".to_string()) so no real-looking secret is committed,
and document in a comment that users should supply their API key via env var or
config.

Comment on lines +11 to +17
let client = RunAgentClient::new(runagent::RunAgentClientConfig {
agent_id: "a6977384-6c88-40dc-a629-e6bf077786ae".to_string(),
entrypoint_tag: "minimal".to_string(),
api_key: Some("rau_b4dcebdef6386726b08971a1cc968d8a2b77c5834d30f3f5a43bddf065cd95cb".to_string()),
base_url: Some("http://localhost:8333/".to_string()),
..runagent::RunAgentClientConfig::default() // Omits all None fields
}).await?;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Remove hard-coded API key-like value here as well

This example also embeds the same rau_... API key-looking literal in the api_key field. That should not be checked into the repo.

Suggested change:

-        api_key: Some("rau_b4dcebdef6386726b08971a1cc968d8a2b77c5834d30f3f5a43bddf065cd95cb".to_string()),
+        api_key: Some("YOUR_RUNAGENT_API_KEY".to_string()),

Consider mirroring whatever solution you adopt in runagent-rust/examples/direct_construction.rs (placeholder or env-based).

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let client = RunAgentClient::new(runagent::RunAgentClientConfig {
agent_id: "a6977384-6c88-40dc-a629-e6bf077786ae".to_string(),
entrypoint_tag: "minimal".to_string(),
api_key: Some("rau_b4dcebdef6386726b08971a1cc968d8a2b77c5834d30f3f5a43bddf065cd95cb".to_string()),
base_url: Some("http://localhost:8333/".to_string()),
..runagent::RunAgentClientConfig::default() // Omits all None fields
}).await?;
let client = RunAgentClient::new(runagent::RunAgentClientConfig {
agent_id: "a6977384-6c88-40dc-a629-e6bf077786ae".to_string(),
entrypoint_tag: "minimal".to_string(),
api_key: Some("YOUR_RUNAGENT_API_KEY".to_string()),
base_url: Some("http://localhost:8333/".to_string()),
..runagent::RunAgentClientConfig::default() // Omits all None fields
}).await?;
🤖 Prompt for AI Agents
In runagent-rust/runagent/examples/direct_construction_omitted.rs around lines
11 to 17, the example contains a hard-coded API key-like literal in the api_key
field; remove this secret and replace it with a non-secret placeholder or load
from an environment variable. Update the code to set api_key to None or to read
from std::env::var("RUNAGENT_API_KEY").ok() (or use the same placeholder/env
pattern used in runagent-rust/examples/direct_construction.rs) and add a brief
comment indicating users should set the RUNAGENT_API_KEY in their environment
for real runs.


## Need Help?

- Docs: `docs/sdk/rust/` (coming soon) or https://docs.rs/runagent
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix bare URL per markdown linting.

The bare URL should be wrapped in angle brackets to comply with markdown standards.

Apply this diff:

-- Docs: `docs/sdk/rust/` (coming soon) or https://docs.rs/runagent
+- Docs: `docs/sdk/rust/` (coming soon) or <https://docs.rs/runagent>
📝 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
- Docs: `docs/sdk/rust/` (coming soon) or https://docs.rs/runagent
- Docs: `docs/sdk/rust/` (coming soon) or <https://docs.rs/runagent>
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)

313-313: Bare URL used

(MD034, no-bare-urls)

🤖 Prompt for AI Agents
In runagent-rust/runagent/README.md around line 313, the markdown contains a
bare URL "https://docs.rs/runagent" which violates markdown linting; update the
line to wrap the URL in angle brackets (e.g. <https://docs.rs/runagent>) so the
link is not a bare URL and complies with the linting rules.

Comment on lines 220 to 279
/// Get agent architecture information
pub async fn get_agent_architecture(&self, agent_id: &str) -> RunAgentResult<Value> {
let path = format!("agents/{}/architecture", agent_id);
self.get(&path).await
.and_then(|response| {
if let Some(success) = response.get("success").and_then(|v| v.as_bool()) {
if success {
if let Some(data) = response.get("data") {
return Ok(data.clone());
}
return Err(RunAgentError::server(
"Architecture response missing data".to_string(),
));
}
let response = self.get(&path).await?;

let message = response
.get("error")
.and_then(|err| {
if err.is_object() {
err.get("message").and_then(|m| m.as_str()).map(|s| s.to_string())
} else {
err.as_str().map(|s| s.to_string())
}
})
.or_else(|| {
response
.get("message")
.and_then(|m| m.as_str())
.map(|s| s.to_string())
})
.unwrap_or_else(|| "Failed to retrieve agent architecture".to_string());

return Err(RunAgentError::server(message));
if let Some(success) = response.get("success").and_then(|v| v.as_bool()) {
if success {
if let Some(data) = response.get("data") {
return Ok(data.clone());
}
return Err(RunAgentError::execution(
"ARCHITECTURE_MISSING",
"Architecture response missing data",
Some("Redeploy the agent or ensure entrypoints are configured.".to_string()),
Some(response),
));
}

Ok(response)
})
.map_err(|e| {
if e.category() == "validation" && e.to_string().contains("Not found") {
RunAgentError::validation(format!(
"Agent {} not found on server. Check that the agent exists and is deployed. Error: {}",
agent_id, e
))
let (code, message, suggestion) = if let Some(error_obj) = response.get("error") {
if let Some(obj) = error_obj.as_object() {
(
obj.get("code")
.and_then(|c| c.as_str())
.unwrap_or("UNKNOWN_ERROR")
.to_string(),
obj.get("message")
.and_then(|m| m.as_str())
.unwrap_or("Failed to retrieve agent architecture")
.to_string(),
obj.get("suggestion")
.and_then(|s| s.as_str())
.map(|s| s.to_string()),
)
} else if let Some(msg) = error_obj.as_str() {
("UNKNOWN_ERROR".to_string(), msg.to_string(), None)
} else {
e
("UNKNOWN_ERROR".to_string(), "Failed to retrieve agent architecture".to_string(), None)
}
})
} else {
(
"UNKNOWN_ERROR".to_string(),
response
.get("message")
.and_then(|m| m.as_str())
.unwrap_or("Failed to retrieve agent architecture")
.to_string(),
None,
)
};

return Err(RunAgentError::execution(
code,
message,
suggestion,
Some(response),
));
}

Ok(response)
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

Execution error mapping for architecture responses is consistent, with a couple of edge cases to consider

The new flow cleanly promotes backend success/error into RunAgentError::execution and includes the full response as details, which is useful for debugging. Two small edge cases you may want to tighten up:

  • For success: true with data: null, the current code treats this as a valid architecture and returns Value::Null. If the API’s contract is “must have a non-null data object,” consider treating Value::Null as missing and emitting ARCHITECTURE_MISSING as well.
  • If a response carries an error field but success is missing or not boolean, it bypasses the execution error branch and returns the raw response. If the API might send such responses, you could optionally fall back to the same (code, message, suggestion) extraction before returning.

These are non-blocking but would make the behavior more robust against slightly malformed responses.


🏁 Script executed:

#!/bin/bash
# Quick scan to see how architecture responses are shaped elsewhere (tests, docs, etc.)
rg -n "architecture" runagent-rust/runagent -S -C3

Length of output: 13285


Tighten data and entrypoints validation to match documented contract

The README states "If data.entrypoints is missing we raise ARCHITECTURE_MISSING," but the current code only checks for a missing data field, not for missing/invalid entrypoints. If the API returns { "success": true, "data": null } or { "success": true, "data": {} }, the code returns Value::Null or an object without entrypoints, which later validation must catch separately with a less clear error message.

Additionally, when success is absent or non-boolean but error is present, the code returns the raw response instead of extracting and propagating the error code/message/suggestion.

To align with the documented contract:

  • Add a check for both missing data field AND missing/invalid entrypoints within data to emit ARCHITECTURE_MISSING early.
  • Optionally, when error is present (regardless of success state), fall back to the error extraction logic before returning raw response.
🤖 Prompt for AI Agents
In runagent-rust/runagent/src/client/rest_client.rs around lines 220 to 279,
tighten the success/data handling: when success is true, validate that
response.get("data") exists AND that data.get("entrypoints") exists and is the
expected type (e.g., array/object per contract); if entrypoints is missing or
invalid return RunAgentError::execution with code "ARCHITECTURE_MISSING" and the
same message/suggestion used for missing data. Also change the branch that
handles absent/non-boolean success to first check for an "error" field and
extract its code/message/suggestion (as done in the existing error branch)
instead of returning the raw response, then return
Err(RunAgentError::execution(..., Some(response))) with the extracted values.

Comment on lines 345 to 384
if response.get("success").and_then(|s| s.as_bool()).unwrap_or(false) {
// Process response data
let mut payload: Option<Value> = None;

if let Some(data) = response.get("data") {
// Simplified payload: data is a structured output string
if let Some(data_str) = data.as_str() {
if let Ok(parsed) = serde_json::from_str::<Value>(data_str) {
return self.serializer.deserialize_object(parsed);
}
return self
.serializer
.deserialize_object(Value::String(data_str.to_string()));
// Case 1: data is a string (simplified payload - could be JSON string with {type, payload})
if data.as_str().is_some() {
// Use common deserializer preparation logic
let prepared = self.serializer.prepare_for_deserialization(data.clone());
payload = Some(prepared);
}

// Legacy detailed execution payload
if let Some(result_data) = data.get("result_data") {
// Case 2: data has result_data.data (legacy detailed execution payload)
else if let Some(result_data) = data.get("result_data") {
if let Some(output_data) = result_data.get("data") {
// Check if the output contains a generator object string
if let Some(content_str) = output_data.as_str() {
if content_str.contains("generator object") {
tracing::warn!("Agent returned generator object instead of content. Consider using streaming endpoint for this agent.");
// Return the raw string for now
return Ok(output_data.clone());
}
// If it's a JSON string, try to parse it first
if let Ok(parsed) = serde_json::from_str::<Value>(content_str) {
return self.serializer.deserialize_object(parsed);
}
}
return self.serializer.deserialize_object(output_data.clone());
payload = Some(output_data.clone());
}
}
// Case 3: data is an object (could be {type, payload} structure)
else if data.is_object() {
payload = Some(data.clone());
}
}
// Fallback to old format for backward compatibility
if let Some(output_data) = response.get("output_data") {
// Check if the output contains a generator object string
if let Some(content_str) = output_data.as_str() {
// Case 4: Fallback to output_data (backward compatibility)
else if let Some(output_data) = response.get("output_data") {
payload = Some(output_data.clone());
}

// Deserialize the payload using serializer (handles {type, payload} structure)
if let Some(payload_val) = payload {
// Check for generator object warning
if let Some(content_str) = payload_val.as_str() {
if content_str.contains("generator object") {
tracing::warn!("Agent returned generator object instead of content. Consider using streaming endpoint for this agent.");
// Return the raw string for now
return Ok(output_data.clone());
}
// If it's a JSON string, try to parse it first
if let Ok(parsed) = serde_json::from_str::<Value>(content_str) {
return self.serializer.deserialize_object(parsed);
return Ok(payload_val);
}
}
return self.serializer.deserialize_object(output_data.clone());
// Deserialize the payload - this should extract payload from {type, payload} structure
let deserialized = self.serializer.deserialize_object(payload_val)?;
return Ok(deserialized);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

Verify the payload handling with multiple response formats.

The extended payload handling now supports four different cases, which is good for backward compatibility. However, the branching logic is complex and would benefit from integration tests to ensure each case works correctly.

Consider adding tests for:

  1. String payload with JSON content
  2. result_data.data nested structure
  3. Direct object payload with {type, payload}
  4. Fallback output_data path

🏁 Script executed:

#!/bin/bash
# Search for existing tests of the payload handling logic
rg -nC5 'test.*payload|test.*deserialize' runagent-rust/runagent/src/client/ runagent-rust/runagent/tests/

Length of output: 69


Let me explore the repository structure to locate tests and verify the payload handling logic.


---

🏁 Script executed:

```shell
#!/bin/bash
# Find all test-related files and directories
fd -t f '(test|spec)' runagent-rust/ --max-depth 5 || echo "No test files found with common patterns"

Length of output: 51


🏁 Script executed:

#!/bin/bash
# Check the actual directory structure of the runagent-rust project
find runagent-rust -type d -name tests -o -type d -name test 2>/dev/null | head -20

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Look for mod.rs files that might contain inline tests
rg -l '#\[cfg\(test\)\]' runagent-rust/ --type rust

Length of output: 355


🏁 Script executed:

#!/bin/bash
# Search for payload and deserialization related tests anywhere in src
rg -n 'payload|deserialize' runagent-rust/runagent/src/client/ --type rust -A2 -B2

Length of output: 6241


Let me check the existing test file and the serializer tests to understand what's currently covered.


---

🏁 Script executed:

```shell
#!/bin/bash
# View the test_deserialize example
cat runagent-rust/runagent/examples/test_deserialize.rs

Length of output: 511


🏁 Script executed:

#!/bin/bash
# Check if runagent_client.rs has any inline test module
rg -n '#\[cfg\(test\)\]' runagent-rust/runagent/src/client/runagent_client.rs

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Extract test sections from serializer.rs
rg -n -A30 '#\[cfg\(test\)\]' runagent-rust/runagent/src/utils/serializer.rs

Length of output: 1061


🏁 Script executed:

#!/bin/bash
# Check for any tests in the client module (check mod.rs if it exists)
cat runagent-rust/runagent/src/client/mod.rs 2>/dev/null || echo "No mod.rs in client directory"

Length of output: 298


Let me check if rest_client.rs or socket_client.rs have tests covering this logic.


---

🏁 Script executed:

```shell
#!/bin/bash
# Extract test sections from rest_client.rs and socket_client.rs
rg -n -A20 '#\[cfg\(test\)\]' runagent-rust/runagent/src/client/rest_client.rs runagent-rust/runagent/src/client/socket_client.rs

Length of output: 3709


🏁 Script executed:

#!/bin/bash
# Search for any integration test files or test modules elsewhere
find runagent-rust -type f -name '*.rs' -path '*/test*' -o -type f -name '*_test.rs' 2>/dev/null | head -20

Length of output: 51


🏁 Script executed:

#!/bin/bash
# Check lib.rs for any integration tests that might cover client logic
rg -n -A10 'mod.*test' runagent-rust/runagent/src/lib.rs | head -40

Length of output: 241


Add tests for the payload handling logic in runagent_client.rs.

Verification confirms no tests exist for the four payload cases. The serializer.rs has basic tests, and test_deserialize.rs only covers the {type, payload} structure (case 3). The following cases lack coverage:

  1. String payload with JSON content (lines 350-355)
  2. result_data.data nested structure (lines 356-361)
  3. Fallback output_data path (lines 368-370)

Add unit or integration tests to ensure each case correctly extracts and deserializes payloads, especially given the multiple legacy format support for backward compatibility.

🤖 Prompt for AI Agents
In runagent-rust/runagent/src/client/runagent_client.rs around lines 345 to 384,
add unit tests that cover the missing payload extraction paths: (1) a response
where "data" is a JSON string and should be prepared via
serializer.prepare_for_deserialization and then deserialized, (2) a response
where "data" contains "result_data" -> "data" and that nested object should be
extracted and deserialized, and (3) a response that falls back to "output_data"
and is deserialized. For each test, construct a fake response Value matching the
case, instantiate a RunAgentClient with a real or test serializer (or a small
test double that records calls), invoke the method that processes the response,
and assert that prepare_for_deserialization is called for the string case, the
nested field is extracted for the result_data case, and output_data is used for
the fallback case, and finally assert the returned Value equals the expected
deserialized output; also include a test covering the "generator object" string
warning path to ensure it returns early. Ensure tests are added under tests or
the appropriate module, run with cargo test, and keep them isolated and
deterministic.

@sawradip sawradip merged commit 2cd3225 into main Nov 17, 2025
2 checks passed
@coderabbitai coderabbitai Bot mentioned this pull request Dec 4, 2025
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant