feat: updating rust sdk to not have exposed connect and work sync#1309
Merged
sergiofilhowz merged 6 commits intomainfrom Mar 14, 2026
Merged
feat: updating rust sdk to not have exposed connect and work sync#1309sergiofilhowz merged 6 commits intomainfrom
sergiofilhowz merged 6 commits intomainfrom
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ytallo
previously approved these changes
Mar 13, 2026
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces async III::new/connect with register_worker(InitOptions) and message-based registration across SDK, engine adapters, and tests; removes builder APIs for TriggerRequest/TriggerAction; makes connect() crate-private; removes task-local Context in favor of Logger; tests use a shared_iii harness. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test
participant SDK as iii_sdk::III
participant Engine as Engine/Bridge
participant Function as TargetFunction
rect rgba(200,220,255,0.5)
Test->>SDK: register_worker(address, InitOptions::default())
SDK->>SDK: create III instance, start background tasks
end
rect rgba(200,255,200,0.5)
Test->>SDK: TriggerRequest { function_id, payload, action?, timeout_ms? }
SDK->>Engine: send TriggerRequest over WebSocket
Engine->>Function: dispatch invoke/enqueue
Function-->>Engine: result / ack
Engine-->>SDK: response
SDK-->>Test: deliver result or acknowledge
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/packages/rust/iii/src/lib.rs (1)
74-101:⚠️ Potential issue | 🟠 MajorUpdate documentation and provide a fallible initialization path.
The function currently panics when no Tokio runtime is found, but the docs (line 62) still advertise
IIIError::Runtimeas the contract, and the example (lines 41-45) demonstrates usage outside a runtime—both now invalid. Keep aResult-returning path (either modifyregister_workerto returnResult<III, IIIError>or add atry_register_workervariant) so embedders can handle missing runtimes gracefully instead of crashing the process.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/packages/rust/iii/src/lib.rs` around lines 74 - 101, register_worker currently panics when no Tokio runtime is present; change it to provide a fallible API that returns Result<III, IIIError> (or add a try_register_worker returning that Result) so callers can handle IIIError::Runtime instead of crashing. Keep the same InitOptions destructuring and construction logic (InitOptions, III::with_metadata, III::new, and otel handling) but replace the tokio::runtime::Handle::try_current() panic branch with an early Err(IIIError::Runtime(...)) return; call iii.connect() only after runtime presence is confirmed. Update the public signature and any call sites and documentation/examples to use the Result-returning API or the new try_register_worker variant.
🧹 Nitpick comments (3)
sdk/packages/rust/iii/tests/common/mod.rs (1)
12-17: Avoid lazy-initializing the SDK during process teardown.
shutdown()currently callsshared_iii(), which can create runtime + worker at shutdown time when tests never used it.♻️ Proposed fix
#[ctor::dtor] fn shutdown() { - let iii = shared_iii(); - - if let Some(rt) = SHARED_RT.get() { - rt.block_on(iii.shutdown_async()); - } + let Some(iii) = SHARED_III.get() else { return }; + if let Some(rt) = SHARED_RT.get() { + rt.block_on(iii.shutdown_async()); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/packages/rust/iii/tests/common/mod.rs` around lines 12 - 17, The shutdown() function should not call shared_iii() (which lazily creates the SDK) during teardown; instead, check for an already-initialized instance and only shut that down. Replace the shared_iii() call with a non-creating lookup (e.g. use SHARED_III.get() or equivalent) and, if it returns Some(iii), call rt.block_on(iii.shutdown_async()); do nothing if the shared instance is absent. Ensure SHARED_RT.get() remains used to avoid creating a runtime.sdk/packages/rust/iii/tests/api_triggers.rs (1)
51-54: Replace fixed sleeps with readiness polling.The repeated
settle + 500ms sleeppattern is brittle and slows this suite. Prefer a small retry loop with timeout against the target endpoint/condition.Also applies to: 89-91, 130-132, 169-171, 206-208, 292-294, 411-413, 516-518, 662-664, 812-814
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/packages/rust/iii/tests/api_triggers.rs` around lines 51 - 54, Replace brittle fixed sleeps (calls to common::settle().await; sleep(Duration::from_millis(...)).await;) with a small readiness-poll helper that retries until a condition or timeout. Implement a helper like async fn wait_for_condition<F, Fut>(mut check: F, timeout: Duration) where check() -> Fut -> Result<bool, E>, using tokio::time::timeout and a short interval (e.g., 50-200ms) loop to call check until it returns true or timeout expires; use this helper to poll the actual readiness condition (HTTP health endpoint, trigger existence, or specific API call used in the test) instead of sleep. Replace each occurrence (the pattern around common::settle() and sleep) with await wait_for_condition(|| async { /* perform the same request/assertion used after the sleep and return true on success */ }, Duration::from_secs(5)).engine/src/modules/state/adapters/bridge.rs (1)
55-60: Consider propagating serialization errors instead of defaulting to null.The
unwrap_or(serde_json::Value::Null)pattern silently converts serialization failures tonull, which could lead to confusing downstream errors (e.g., the bridge receivingnullinstead of the expected payload). While the input structs deriveSerializeand serialization failures are unlikely, propagating the error would be more robust.♻️ Optional: Propagate serialization error
let result = self .bridge .trigger(TriggerRequest { function_id: "state::update".to_string(), - payload: serde_json::to_value(data).unwrap_or(serde_json::Value::Null), + payload: serde_json::to_value(data) + .map_err(|e| anyhow::anyhow!("Failed to serialize update input: {}", e))?, action: None, timeout_ms: None, })Apply the same pattern to the other
unwrap_or(Value::Null)usages at lines 83, 103, 122, 140, and 156.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/modules/state/adapters/bridge.rs` around lines 55 - 60, The code currently uses serde_json::to_value(...).unwrap_or(serde_json::Value::Null) when building the TriggerRequest (e.g., the call that sets function_id "state::update"), which silently converts serialization failures to null; change this to propagate the serialization error instead: call serde_json::to_value(...) and map or ? the Result so the surrounding function returns a Result/Error (or map the serde error into your adapter's error type) rather than defaulting to Value::Null; apply the same change to the other unwrap_or(Value::Null) occurrences used when constructing TriggerRequest payloads so all serialization failures are returned to the caller instead of producing a null payload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/src/modules/queue/adapters/bridge.rs`:
- Around line 71-75: BridgeAdapter::new currently always returns Ok even when
register_worker fails; update it so connection errors are detected and bubbled
up: change the call to register_worker(&bridge_url, InitOptions::default()) to a
fallible operation (handle its Result or change register_worker to return
anyhow::Result), validate the bridge_url (e.g., parse it) and/or perform an
immediate connection/handshake check, and return Err(...) from
BridgeAdapter::new when that check fails instead of always returning Ok(Self {
bridge, ... }); reference BridgeAdapter::new, register_worker, and
InitOptions::default when locating where to add the error handling and return
the error via anyhow::Result.
In `@sdk/packages/rust/iii/src/iii.rs`:
- Around line 374-383: The WebSocket task (run_connection) must be gated on OTel
initialization so early messages get span context; change the startup so
telemetry::init_otel(config) completes (or signals readiness) before spawning
run_connection, e.g. create a oneshot/watcher that telemetry::init_otel sends
when ready and pass that readiness handle into run_connection (or await it
before calling tokio::spawn for run_connection); update references around
self.inner.otel_config.lock_or_recover(), telemetry::init_otel, and
run_connection so run_connection waits on the readiness signal (or is spawned
after init_otel finishes) to ensure propagation headers and spans are present.
- Around line 359-365: The change made connect() private which breaks the public
API because III::new and III::with_metadata are public but callers cannot finish
initialization; either restore the original public API by making connect() pub
(change fn connect(&self) to pub fn connect(&self)) so external users can call
it after III::new/III::with_metadata, or if you intend register_worker() to be
the only initialization path, make the constructors (III::new and
III::with_metadata) crate-private (pub(crate)) and update the III docstring to
remove or clarify the example; reference the symbols III::connect, III::new,
III::with_metadata, and register_worker when applying the change.
In `@sdk/packages/rust/iii/src/stream.rs`:
- Around line 8-10: The example docs incorrectly use the `?` operator on the
non-Result return of `register_worker`; remove the trailing `?` so the call is
`register_worker("ws://localhost:49134", InitOptions::default())` (reference the
`register_worker` call and `InitOptions::default()` in the example) to make the
example compile.
In `@sdk/packages/rust/iii/tests/bridge.rs`:
- Around line 80-86: The test uses the enum variant TriggerAction::Void
directly; change it to the preferred constructor method TriggerAction::void()
for consistency with other tests and docs—update the TriggerRequest construction
(where TriggerAction::Void is used) to call TriggerAction::void() so the payload
creation in iii.trigger(...) uses the method form.
In `@sdk/packages/rust/iii/tests/init_api.rs`:
- Around line 1-6: Add a new test that explicitly covers the "no Tokio runtime"
panic path by creating a plain synchronous test (no #[tokio::test]) annotated
with #[should_panic] which calls register_worker("ws://127.0.0.1:49134",
InitOptions::default()); ensure the test function name clearly indicates the
case (e.g., init_without_runtime_panics) and imports InitOptions and
register_worker so the panic contract in register_worker is exercised and
verified.
---
Outside diff comments:
In `@sdk/packages/rust/iii/src/lib.rs`:
- Around line 74-101: register_worker currently panics when no Tokio runtime is
present; change it to provide a fallible API that returns Result<III, IIIError>
(or add a try_register_worker returning that Result) so callers can handle
IIIError::Runtime instead of crashing. Keep the same InitOptions destructuring
and construction logic (InitOptions, III::with_metadata, III::new, and otel
handling) but replace the tokio::runtime::Handle::try_current() panic branch
with an early Err(IIIError::Runtime(...)) return; call iii.connect() only after
runtime presence is confirmed. Update the public signature and any call sites
and documentation/examples to use the Result-returning API or the new
try_register_worker variant.
---
Nitpick comments:
In `@engine/src/modules/state/adapters/bridge.rs`:
- Around line 55-60: The code currently uses
serde_json::to_value(...).unwrap_or(serde_json::Value::Null) when building the
TriggerRequest (e.g., the call that sets function_id "state::update"), which
silently converts serialization failures to null; change this to propagate the
serialization error instead: call serde_json::to_value(...) and map or ? the
Result so the surrounding function returns a Result/Error (or map the serde
error into your adapter's error type) rather than defaulting to Value::Null;
apply the same change to the other unwrap_or(Value::Null) occurrences used when
constructing TriggerRequest payloads so all serialization failures are returned
to the caller instead of producing a null payload.
In `@sdk/packages/rust/iii/tests/api_triggers.rs`:
- Around line 51-54: Replace brittle fixed sleeps (calls to
common::settle().await; sleep(Duration::from_millis(...)).await;) with a small
readiness-poll helper that retries until a condition or timeout. Implement a
helper like async fn wait_for_condition<F, Fut>(mut check: F, timeout: Duration)
where check() -> Fut -> Result<bool, E>, using tokio::time::timeout and a short
interval (e.g., 50-200ms) loop to call check until it returns true or timeout
expires; use this helper to poll the actual readiness condition (HTTP health
endpoint, trigger existence, or specific API call used in the test) instead of
sleep. Replace each occurrence (the pattern around common::settle() and sleep)
with await wait_for_condition(|| async { /* perform the same request/assertion
used after the sleep and return true on success */ }, Duration::from_secs(5)).
In `@sdk/packages/rust/iii/tests/common/mod.rs`:
- Around line 12-17: The shutdown() function should not call shared_iii() (which
lazily creates the SDK) during teardown; instead, check for an
already-initialized instance and only shut that down. Replace the shared_iii()
call with a non-creating lookup (e.g. use SHARED_III.get() or equivalent) and,
if it returns Some(iii), call rt.block_on(iii.shutdown_async()); do nothing if
the shared instance is absent. Ensure SHARED_RT.get() remains used to avoid
creating a runtime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a6aefa51-7be2-4e22-adc8-60b4015a7c73
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
engine/src/modules/bridge_client/mod.rsengine/src/modules/observability/mod.rsengine/src/modules/queue/adapters/bridge.rsengine/src/modules/state/adapters/bridge.rsengine/src/modules/stream/adapters/bridge.rssdk/packages/rust/iii-example/src/main.rssdk/packages/rust/iii/Cargo.tomlsdk/packages/rust/iii/src/iii.rssdk/packages/rust/iii/src/lib.rssdk/packages/rust/iii/src/protocol.rssdk/packages/rust/iii/src/stream.rssdk/packages/rust/iii/tests/api_triggers.rssdk/packages/rust/iii/tests/bridge.rssdk/packages/rust/iii/tests/common/mod.rssdk/packages/rust/iii/tests/data_channels.rssdk/packages/rust/iii/tests/healthcheck.rssdk/packages/rust/iii/tests/http_external_functions.rssdk/packages/rust/iii/tests/init_api.rssdk/packages/rust/iii/tests/pubsub.rssdk/packages/rust/iii/tests/queue_integration.rssdk/packages/rust/iii/tests/state.rssdk/packages/rust/iii/tests/stream.rs
Comment on lines
71
to
75
| pub async fn new(engine: Arc<Engine>, bridge_url: String) -> anyhow::Result<Self> { | ||
| tracing::info!(bridge_url = %bridge_url, "Connecting to bridge"); | ||
|
|
||
| let bridge = Arc::new(III::new(&bridge_url)); | ||
| bridge | ||
| .connect() | ||
| .await | ||
| .map_err(|e| anyhow::anyhow!("{}", e))?; | ||
| let bridge = Arc::new(register_worker(&bridge_url, InitOptions::default())); | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify constructor contract vs register_worker behavior.
rg -n "pub async fn new\\(engine: Arc<Engine>, bridge_url: String\\) -> anyhow::Result<Self>|Returns error if bridge connection fails|register_worker\\(" engine/src/modules/queue/adapters/bridge.rs -C3
rg -n "pub fn register_worker\\(|Handle::try_current|panic!|iii.connect\\(" sdk/packages/rust/iii/src/lib.rs -C4Repository: iii-hq/iii
Length of output: 1106
BridgeAdapter::new always succeeds despite documentation promising error reporting on connection failure.
The constructor documentation (line 70) states "Returns error if bridge connection fails," but register_worker() returns III directly (not Result), and BridgeAdapter::new always returns Ok(Self { ... }) on line 76 regardless of whether the bridge URL is valid or the connection succeeds. Invalid URLs or connection failures will not surface until later when the bridge is first used.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/src/modules/queue/adapters/bridge.rs` around lines 71 - 75,
BridgeAdapter::new currently always returns Ok even when register_worker fails;
update it so connection errors are detected and bubbled up: change the call to
register_worker(&bridge_url, InitOptions::default()) to a fallible operation
(handle its Result or change register_worker to return anyhow::Result), validate
the bridge_url (e.g., parse it) and/or perform an immediate connection/handshake
check, and return Err(...) from BridgeAdapter::new when that check fails instead
of always returning Ok(Self { bridge, ... }); reference BridgeAdapter::new,
register_worker, and InitOptions::default when locating where to add the error
handling and return the error via anyhow::Result.
Comment on lines
374
to
+383
| #[cfg(feature = "otel")] | ||
| { | ||
| let config = self.inner.otel_config.lock_or_recover().take(); | ||
| if let Some(mut config) = config { | ||
| // Default engine_ws_url to the III address if not set | ||
| if config.engine_ws_url.is_none() { | ||
| config.engine_ws_url = Some(self.inner.address.clone()); | ||
| } | ||
| telemetry::init_otel(config).await; | ||
| tokio::spawn(async move { | ||
| telemetry::init_otel(config).await; | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the full context around lines 374-383
cd sdk/packages/rust/iii/src && wc -l iii.rsRepository: iii-hq/iii
Length of output: 65
🏁 Script executed:
# Get the context around the mentioned lines
sed -n '360,395p' sdk/packages/rust/iii/src/iii.rs | cat -nRepository: iii-hq/iii
Length of output: 1593
🏁 Script executed:
# Search for run_connection in the file to understand when it's spawned
rg -n "run_connection" sdk/packages/rust/iii/src/iii.rsRepository: iii-hq/iii
Length of output: 184
🏁 Script executed:
# Look for the broader function context - find the function containing these lines
sed -n '300,383p' sdk/packages/rust/iii/src/iii.rs | cat -nRepository: iii-hq/iii
Length of output: 3755
🏁 Script executed:
# Look at the run_connection function implementation starting at line 1015
sed -n '1015,1060p' sdk/packages/rust/iii/src/iii.rs | cat -nRepository: iii-hq/iii
Length of output: 2750
🏁 Script executed:
# Check if there's any synchronization or readiness signal related to OTel
rg -n "otel|telemetry" sdk/packages/rust/iii/src/iii.rs | head -20Repository: iii-hq/iii
Length of output: 975
🏁 Script executed:
# Look for any waiting or blocking mechanisms in the connection flow
sed -n '1015,1150p' sdk/packages/rust/iii/src/iii.rs | cat -nRepository: iii-hq/iii
Length of output: 6661
🏁 Script executed:
# Quick check to see if there are any synchronization primitives or barriers for OTel readiness
rg -n "Barrier|broadcast|notify|Signal|Ready" sdk/packages/rust/iii/src/iii.rs | head -10Repository: iii-hq/iii
Length of output: 104
Gate the connection on OTel initialization to prevent losing telemetry.
run_connection is spawned at line 71, while telemetry::init_otel is spawned in a separate task at line 82. Since both are independent tasks, the WebSocket connection can start processing messages (see tokio::select! at line 1033 in run_connection) before OTel initialization completes. This causes early inbound/outbound activity to lack proper span context and propagation headers.
Add explicit synchronization between these two initialization steps—either by spawning run_connection only after OTel is ready, or by introducing an explicit readiness signal that run_connection waits on before handling messages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/packages/rust/iii/src/iii.rs` around lines 374 - 383, The WebSocket task
(run_connection) must be gated on OTel initialization so early messages get span
context; change the startup so telemetry::init_otel(config) completes (or
signals readiness) before spawning run_connection, e.g. create a oneshot/watcher
that telemetry::init_otel sends when ready and pass that readiness handle into
run_connection (or await it before calling tokio::spawn for run_connection);
update references around self.inner.otel_config.lock_or_recover(),
telemetry::init_otel, and run_connection so run_connection waits on the
readiness signal (or is spawned after init_otel finishes) to ensure propagation
headers and spans are present.
Comment on lines
+8
to
+10
| //! use iii_sdk::{register_worker, InitOptions, Streams, UpdateOp}; | ||
| //! | ||
| //! let iii = III::new("ws://localhost:49134"); | ||
| //! iii.connect().await?; | ||
| //! let iii = register_worker("ws://localhost:49134", InitOptions::default())?; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the files mentioned in the review
find . -type f -name "lib.rs" | grep -E "sdk/packages/rust/iii"
find . -type f -name "stream.rs" | grep -E "sdk/packages/rust/iii"Repository: iii-hq/iii
Length of output: 166
🏁 Script executed:
#!/bin/bash
# Check the stream.rs file around lines 8-10
cat -n sdk/packages/rust/iii/src/stream.rs | head -20Repository: iii-hq/iii
Length of output: 835
🏁 Script executed:
#!/bin/bash
# Search for the register_worker function definition in lib.rs
rg "fn register_worker" sdk/packages/rust/iii/src/lib.rs -A 5Repository: iii-hq/iii
Length of output: 226
Remove the trailing ? operator from the example.
The register_worker function returns III directly, not Result<III, _>. The current example will not compile because the trailing ? operator is invalid for non-Result types.
📝 Suggested doc fix
-//! let iii = register_worker("ws://localhost:49134", InitOptions::default())?;
+//! let iii = register_worker("ws://localhost:49134", InitOptions::default());📝 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
| //! use iii_sdk::{register_worker, InitOptions, Streams, UpdateOp}; | |
| //! | |
| //! let iii = III::new("ws://localhost:49134"); | |
| //! iii.connect().await?; | |
| //! let iii = register_worker("ws://localhost:49134", InitOptions::default())?; | |
| //! use iii_sdk::{register_worker, InitOptions, Streams, UpdateOp}; | |
| //! | |
| //! let iii = register_worker("ws://localhost:49134", InitOptions::default()); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/packages/rust/iii/src/stream.rs` around lines 8 - 10, The example docs
incorrectly use the `?` operator on the non-Result return of `register_worker`;
remove the trailing `?` so the call is `register_worker("ws://localhost:49134",
InitOptions::default())` (reference the `register_worker` call and
`InitOptions::default()` in the example) to make the example compile.
Comment on lines
80
to
+86
| let result = iii | ||
| .trigger( | ||
| TriggerRequest::new("test.bridge.rs.receiver", json!({"value": 42})) | ||
| .action(TriggerAction::Void), | ||
| ) | ||
| .trigger(TriggerRequest { | ||
| function_id: "test.bridge.rs.receiver".to_string(), | ||
| payload: json!({"value": 42}), | ||
| action: Some(TriggerAction::Void), | ||
| timeout_ms: None, | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check TriggerAction definition to verify both Void variant and void() method exist
# Expect: Either both forms are valid, or one is incorrect
# Search for TriggerAction definition
ast-grep --pattern $'enum TriggerAction {
$$$
}'
# Search for void method on TriggerAction
rg -n "fn void\s*\(" --type rust
# Search for all usages in tests
rg -n "TriggerAction::" --type rust -g '*tests*'Repository: iii-hq/iii
Length of output: 288
🏁 Script executed:
# Find TriggerAction definition
rg -n "enum TriggerAction" --type rust -A 15
# Check the specific line in bridge.rs
sed -n '80,90p' sdk/packages/rust/iii/tests/bridge.rs
# Check queue_integration.rs for comparison
rg -n "TriggerAction::void" --type rust -B 2 -A 2
# Also check for TriggerAction::Void usage
rg -n "TriggerAction::Void" --type rust -B 2 -A 2Repository: iii-hq/iii
Length of output: 6597
Use consistent TriggerAction::void() method form in tests.
Both TriggerAction::Void (enum variant) and TriggerAction::void() (constructor method) are valid, but the codebase shows a clear preference for the method form. This test uses TriggerAction::Void while queue_integration.rs uses TriggerAction::void(), and the latter is the pattern used in documentation examples and production code modules. Align with TriggerAction::void() for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/packages/rust/iii/tests/bridge.rs` around lines 80 - 86, The test uses
the enum variant TriggerAction::Void directly; change it to the preferred
constructor method TriggerAction::void() for consistency with other tests and
docs—update the TriggerRequest construction (where TriggerAction::Void is used)
to call TriggerAction::void() so the payload creation in iii.trigger(...) uses
the method form.
Comment on lines
1
to
6
| use iii_sdk::{InitOptions, register_worker}; | ||
|
|
||
| #[tokio::test] | ||
| async fn init_with_runtime_returns_sdk_instance() { | ||
| let client = register_worker("ws://127.0.0.1:49134", InitOptions::default()) | ||
| .expect("register_worker should succeed inside Tokio runtime"); | ||
|
|
||
| let client = register_worker("ws://127.0.0.1:49134", InitOptions::default()); | ||
| // API should remain usable immediately after register_worker() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current SDK contract and missing panic-path coverage in init_api tests.
rg -n "pub fn register_worker\\(|Handle::try_current|panic!\\(" sdk/packages/rust/iii/src/lib.rs -C4
rg -n "should_panic|no runtime|register_worker" sdk/packages/rust/iii/tests/init_api.rs -C3Repository: iii-hq/iii
Length of output: 1246
🏁 Script executed:
wc -l sdk/packages/rust/iii/tests/init_api.rsRepository: iii-hq/iii
Length of output: 96
🏁 Script executed:
cat -n sdk/packages/rust/iii/tests/init_api.rsRepository: iii-hq/iii
Length of output: 1096
🏁 Script executed:
# Check if there are other test files that might cover the no-runtime panic case
fd -e rs sdk/packages/rust/iii/tests/ --type fRepository: iii-hq/iii
Length of output: 471
🏁 Script executed:
fd --full-path 'sdk/packages/rust/iii/tests/' -type f -name '*.rs'Repository: iii-hq/iii
Length of output: 224
🏁 Script executed:
# Search for any #[should_panic] tests across Rust tests
rg -l "should_panic" sdk/packages/rust/iii/tests/Repository: iii-hq/iii
Length of output: 36
🏁 Script executed:
fd '\.rs$' sdk/packages/rust/iii/tests/ --type fRepository: iii-hq/iii
Length of output: 522
🏁 Script executed:
# Search for any panic or runtime error testing patterns across tests
rg "panic|Handle::try_current|no runtime" sdk/packages/rust/iii/tests/ -A2 -B2Repository: iii-hq/iii
Length of output: 2704
Add explicit coverage for the "no Tokio runtime" register_worker panic path.
The test suite currently validates only runtime-present scenarios. Since register_worker explicitly panics when no active Tokio runtime exists (see lib.rs lines 92-96), add a #[should_panic] test to ensure this contract is covered by regression testing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/packages/rust/iii/tests/init_api.rs` around lines 1 - 6, Add a new test
that explicitly covers the "no Tokio runtime" panic path by creating a plain
synchronous test (no #[tokio::test]) annotated with #[should_panic] which calls
register_worker("ws://127.0.0.1:49134", InitOptions::default()); ensure the test
function name clearly indicates the case (e.g., init_without_runtime_panics) and
imports InitOptions and register_worker so the panic contract in register_worker
is exercised and verified.
guibeira
previously approved these changes
Mar 13, 2026
… and remove deprecated methods - Replaced `III::new()` and `connect().await` with `register_worker()` for automatic WebSocket connection. - Updated API documentation and examples to reflect the new initialization method. - Removed deprecated `connect` method and related context handling. - Adjusted trigger request syntax for clarity and consistency. - Cleaned up logger implementation by removing unused function name attribute.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docs/scripts/parsers/parse-rustdoc.mts (1)
107-119:⚠️ Potential issue | 🟠 MajorThe generated
register_workerdocs are still inconsistent with the new API.The example now calls
register_worker()outside any Tokio runtime, butregister_worker()panics in that case. The same block also says the signature is-> IIIwhile the returns metadata still saysResult<III, IIIError>, so regenerating docs will keep producing conflicting instructions.📝 Suggested update
initialization: { - description: 'The Rust SDK provides `register_worker()` to create a connected SDK instance. The WebSocket connection is established automatically.', - example: `use iii_sdk::{register_worker, InitOptions};\n\nlet iii = register_worker("ws://localhost:49134", InitOptions::default());`, + description: 'The Rust SDK provides `register_worker()` to create an auto-connecting SDK instance. Call it from inside an active Tokio runtime.', + example: `use iii_sdk::{register_worker, InitOptions};\n\n#[tokio::main]\nasync fn main() {\n let iii = register_worker("ws://localhost:49134", InitOptions::default());\n}`, entryPoint: { name: 'register_worker', signature: '(address: &str, options: InitOptions) -> III', description: 'Create and return a connected SDK instance.', @@ - returns: { type: 'Result<III, IIIError>', description: 'Connected SDK instance.' }, + returns: { type: 'III', description: 'Auto-connecting SDK instance.' },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/scripts/parsers/parse-rustdoc.mts` around lines 107 - 119, The docs for register_worker are inconsistent and unsafe: the example calls register_worker() outside a Tokio runtime (which causes a panic) and the entryPoint signature shows '-> III' while the returns block says 'Result<III, IIIError>'; update the example to invoke register_worker within an async runtime (e.g., mention using Tokio or show awaiting inside a #[tokio::main] context) or otherwise demonstrate proper error handling, and make the entryPoint signature and returns metadata consistent by changing the signature to return Result<III, IIIError> (or change the returns to III if the function actually panics/unwinds) so register_worker, InitOptions, III and IIIError are all referenced consistently.sdk/packages/rust/iii/tests/queue_integration.rs (1)
18-29:⚠️ Potential issue | 🟠 MajorThis shared harness makes the queue tests order-dependent.
common::shared_iii()is process-wide, these registrations drop the returnedFunctionRefinstead of unregistering, andcommon::settle()is only a fixed 300ms sleep. That means handlers and queued work can leak across tests, while slower CI can still race the trigger before registration has propagated. I’d keep these tests isolated per test case, or at least use unique IDs plus polling instead of fixed sleeps. This same pattern repeats through the rest of the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/packages/rust/iii/tests/queue_integration.rs` around lines 18 - 29, Tests currently reuse the process-wide instance from common::shared_iii(), drop the returned FunctionRef from register_function (so handlers stay registered), and rely on common::settle() (fixed sleep), causing cross-test leakage and race conditions; fix by making each test isolate its handler: create or obtain a fresh iii per test (avoid shared_iii) or register using a unique function name (e.g., include test-specific UUID/test-id) and keep the returned FunctionRef so you can explicitly unregister or drop it deterministically after the test, and replace common::settle() with a polling loop that checks the received Arc<Mutex<Vec<_>>> (or uses an explicit unregister/ack API) with a timeout to wait for expected messages instead of sleeping, referencing register_function, FunctionRef, common::shared_iii, and common::settle.docs/content/api-reference/sdk-rust.mdx (1)
1361-1373:⚠️ Potential issue | 🟠 MajorThe logger snippet is stale against the new Rust SDK API.
Logger::new(Some(...))was removed in this PR, andlogger.infonow takes thedataargument, so this example no longer compiles. Because this file is generated, the corresponding template/parser needs the same update.📝 Corrected snippet
use iii_sdk::Logger; -let logger = Logger::new(Some("my-function".to_string())); -logger.info("Processing started"); +let logger = Logger::new(); +logger.info("Processing started", None);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/api-reference/sdk-rust.mdx` around lines 1361 - 1373, Update the generated Rust snippet and its template/parser to use the new Logger API: replace the removed Logger::new(Some(...)) call with the new constructor (e.g., Logger::new() or the current zero-arg form used by the SDK) and update logger.info invocations to pass the required data argument (e.g., logger.info("Processing started") -> logger.info(<data>) using the SDK's current parameter form). Ensure the changes are applied to the template that emits the example near register_worker / InitOptions so future generation uses the updated Logger construction and the new logger.info signature.sdk/packages/rust/iii/src/protocol.rs (1)
62-69:⚠️ Potential issue | 🔴 CriticalMigrate
TriggerActionhelper call sites or add compatibility shims before removing the helper methods.The removal of
TriggerAction::void()andTriggerAction::enqueue()breaks 3 existing call sites in the workspace:
engine/src/modules/queue/adapters/bridge.rs:141—iii_sdk::TriggerAction::void()engine/src/modules/queue/adapters/bridge.rs:321—TriggerAction::enqueue(queue_name)engine/src/modules/bridge_client/mod.rs:175—TriggerAction::void()Either migrate these callers to use the enum variants directly (
TriggerAction::Void,TriggerAction::Enqueue { queue: ... }) in this PR, or add deprecated shim methods to maintain compatibility for one release.♻️ Compatibility shim option
+impl TriggerAction { + #[deprecated(note = "Use TriggerAction::Void")] + pub fn void() -> Self { + Self::Void + } + + #[deprecated(note = "Use TriggerAction::Enqueue { queue: ... }")] + pub fn enqueue(queue: impl Into<String>) -> Self { + Self::Enqueue { + queue: queue.into(), + } + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/packages/rust/iii/src/protocol.rs` around lines 62 - 69, Call sites expecting helper constructors for TriggerAction were broken when the helper methods were removed; either update callers to use the enum variants directly (TriggerAction::Void and TriggerAction::Enqueue { queue: your_queue }) or add back deprecated shim constructors on the TriggerAction type. To add shims, implement an impl TriggerAction block with pub fn void() -> Self { TriggerAction::Void } and pub fn enqueue<Q: Into<String>>(queue: Q) -> Self { TriggerAction::Enqueue { queue: queue.into() } } annotated as #[deprecated] so existing calls (e.g., places calling TriggerAction::void() and TriggerAction::enqueue(...)) continue to work until callers are migrated.
♻️ Duplicate comments (2)
sdk/packages/rust/iii/src/iii.rs (2)
294-302:⚠️ Potential issue | 🟠 Major
III::new()andIII::with_metadata()are still public even though external callers can't finish initialization.With
connect()now crate-private, another crate can still constructIIIbut has no public way to start the connection loop. Calls likeregister_function()andtrigger()then operate on an instance that never flips torunning. Ifregister_worker()is the only supported entry point now, these constructors should stop being public.🔒 Narrow fix if `register_worker()` is the intended API
- pub fn new(address: &str) -> Self { + pub(crate) fn new(address: &str) -> Self { Self::with_metadata(address, WorkerMetadata::default()) } @@ - pub fn with_metadata(address: &str, metadata: WorkerMetadata) -> Self { + pub(crate) fn with_metadata(address: &str, metadata: WorkerMetadata) -> Self {Also applies to: 346-349
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/packages/rust/iii/src/iii.rs` around lines 294 - 302, The constructors III::new and III::with_metadata are public but callers can't finish initialization because connect() is crate-private; change their visibility to crate-only (e.g., pub(crate) or remove pub) so external crates cannot construct an unusable III, and apply the same visibility change to the other public constructors/ctors mentioned around the 346-349 block; keep register_worker() (the intended public entrypoint) public so external users must go through the supported API.
356-370:⚠️ Potential issue | 🟠 MajorOTel initialization still races behind the connection task.
run_connection()is spawned beforetelemetry::init_otel(), so the initial connect/register flow can emit traffic before tracing/log export is ready. That drops the very first spans/log context again. Please gaterun_connection()on telemetry readiness or initialize telemetry first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/packages/rust/iii/src/iii.rs` around lines 356 - 370, The telemetry init is started after spawning the connection task, causing a race where run_connection() may emit spans before telemetry is ready; move or gate spawning of run_connection so telemetry::init_otel(config).await completes (or readiness is awaited) before calling self.clone() and tokio::spawn(async move { iii.run_connection(rx).await; }); specifically: take the otel config from self.inner.otel_config.lock_or_recover(), set engine_ws_url if missing, await telemetry::init_otel(config) first (or await a returned readiness/future), and only then clone self and spawn run_connection(); keep the same config handling and tokio::spawn usage but invert the order to ensure telemetry is initialized prior to run_connection().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk/packages/rust/iii/README.md`:
- Line 93: Replace the heading/text that currently reads "Synchronous -- waits
for the result" with clearer async-friendly terminology (e.g., "Request/response
(waits for result)" or "Request/response — awaits result") in the README.md;
locate the exact string "Synchronous -- waits for the result" and update it so
it no longer implies blocking/synchronous behavior while the examples still use
.await.
- Line 178: The README table entry incorrectly lists the import path as
`iii_sdk::stream`; update the table to show the idiomatic crate-root import
`iii_sdk` for the stream client (e.g., "`iii_sdk` | Stream client (`Streams`,
`UpdateBuilder`)") or, if you intend to show both, list both paths (e.g.,
"`iii_sdk` / `iii_sdk::stream` | Stream client (`Streams`, `UpdateBuilder`)")
and ensure the description references the re-exported symbols `Streams` and
`UpdateBuilder` (these are re-exported at the crate root).
In `@sdk/packages/rust/iii/src/lib.rs`:
- Around line 39-43: The docs claim register_worker returns an IIIError::Runtime
on missing Tokio, but the current implementation panics; restore a fallible API
or clearly document the panic. Fix by making register_worker a fallible
constructor (e.g., return Result<..., IIIError> or add try_register_worker that
returns Result and have register_worker call it) so callers can handle
IIIError::Runtime instead of crashing; update the doc examples and doc comments
(the code block showing register_worker and any references to IIIError::Runtime)
to show the Result return path (or, if you choose to keep the panic, add a clear
"# Panics" doc and ensure examples run inside a Tokio runtime). Ensure you
reference and adjust the register_worker symbol and the IIIError::Runtime
variant accordingly.
---
Outside diff comments:
In `@docs/content/api-reference/sdk-rust.mdx`:
- Around line 1361-1373: Update the generated Rust snippet and its
template/parser to use the new Logger API: replace the removed
Logger::new(Some(...)) call with the new constructor (e.g., Logger::new() or the
current zero-arg form used by the SDK) and update logger.info invocations to
pass the required data argument (e.g., logger.info("Processing started") ->
logger.info(<data>) using the SDK's current parameter form). Ensure the changes
are applied to the template that emits the example near register_worker /
InitOptions so future generation uses the updated Logger construction and the
new logger.info signature.
In `@docs/scripts/parsers/parse-rustdoc.mts`:
- Around line 107-119: The docs for register_worker are inconsistent and unsafe:
the example calls register_worker() outside a Tokio runtime (which causes a
panic) and the entryPoint signature shows '-> III' while the returns block says
'Result<III, IIIError>'; update the example to invoke register_worker within an
async runtime (e.g., mention using Tokio or show awaiting inside a
#[tokio::main] context) or otherwise demonstrate proper error handling, and make
the entryPoint signature and returns metadata consistent by changing the
signature to return Result<III, IIIError> (or change the returns to III if the
function actually panics/unwinds) so register_worker, InitOptions, III and
IIIError are all referenced consistently.
In `@sdk/packages/rust/iii/src/protocol.rs`:
- Around line 62-69: Call sites expecting helper constructors for TriggerAction
were broken when the helper methods were removed; either update callers to use
the enum variants directly (TriggerAction::Void and TriggerAction::Enqueue {
queue: your_queue }) or add back deprecated shim constructors on the
TriggerAction type. To add shims, implement an impl TriggerAction block with pub
fn void() -> Self { TriggerAction::Void } and pub fn enqueue<Q:
Into<String>>(queue: Q) -> Self { TriggerAction::Enqueue { queue: queue.into() }
} annotated as #[deprecated] so existing calls (e.g., places calling
TriggerAction::void() and TriggerAction::enqueue(...)) continue to work until
callers are migrated.
In `@sdk/packages/rust/iii/tests/queue_integration.rs`:
- Around line 18-29: Tests currently reuse the process-wide instance from
common::shared_iii(), drop the returned FunctionRef from register_function (so
handlers stay registered), and rely on common::settle() (fixed sleep), causing
cross-test leakage and race conditions; fix by making each test isolate its
handler: create or obtain a fresh iii per test (avoid shared_iii) or register
using a unique function name (e.g., include test-specific UUID/test-id) and keep
the returned FunctionRef so you can explicitly unregister or drop it
deterministically after the test, and replace common::settle() with a polling
loop that checks the received Arc<Mutex<Vec<_>>> (or uses an explicit
unregister/ack API) with a timeout to wait for expected messages instead of
sleeping, referencing register_function, FunctionRef, common::shared_iii, and
common::settle.
---
Duplicate comments:
In `@sdk/packages/rust/iii/src/iii.rs`:
- Around line 294-302: The constructors III::new and III::with_metadata are
public but callers can't finish initialization because connect() is
crate-private; change their visibility to crate-only (e.g., pub(crate) or remove
pub) so external crates cannot construct an unusable III, and apply the same
visibility change to the other public constructors/ctors mentioned around the
346-349 block; keep register_worker() (the intended public entrypoint) public so
external users must go through the supported API.
- Around line 356-370: The telemetry init is started after spawning the
connection task, causing a race where run_connection() may emit spans before
telemetry is ready; move or gate spawning of run_connection so
telemetry::init_otel(config).await completes (or readiness is awaited) before
calling self.clone() and tokio::spawn(async move { iii.run_connection(rx).await;
}); specifically: take the otel config from
self.inner.otel_config.lock_or_recover(), set engine_ws_url if missing, await
telemetry::init_otel(config) first (or await a returned readiness/future), and
only then clone self and spawn run_connection(); keep the same config handling
and tokio::spawn usage but invert the order to ensure telemetry is initialized
prior to run_connection().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f9b87ed-cd0d-4402-9066-ce7a0295cd59
📒 Files selected for processing (10)
docs/content/api-reference/sdk-rust.mdxdocs/scripts/parsers/parse-rustdoc.mtssdk/packages/rust/iii-example/src/http_example.rssdk/packages/rust/iii/README.mdsdk/packages/rust/iii/src/context.rssdk/packages/rust/iii/src/iii.rssdk/packages/rust/iii/src/lib.rssdk/packages/rust/iii/src/logger.rssdk/packages/rust/iii/src/protocol.rssdk/packages/rust/iii/tests/queue_integration.rs
💤 Files with no reviewable changes (1)
- sdk/packages/rust/iii/src/context.rs
| .action(TriggerAction::void()), | ||
| ) | ||
| .await?; | ||
| // Synchronous -- waits for the result |
Contributor
There was a problem hiding this comment.
Use async terminology instead of “Synchronous.”
Line 93 says “Synchronous,” but this is still async request/response (.await). Consider “Request/response (waits for result)” to avoid confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/packages/rust/iii/README.md` at line 93, Replace the heading/text that
currently reads "Synchronous -- waits for the result" with clearer
async-friendly terminology (e.g., "Request/response (waits for result)" or
"Request/response — awaits result") in the README.md; locate the exact string
"Synchronous -- waits for the result" and update it so it no longer implies
blocking/synchronous behavior while the examples still use .await.
| | Import | What it provides | | ||
| | -------------------- | ----------------------------------------------------------- | | ||
| | `iii_sdk` | Core SDK (`III`, `register_worker`, `TriggerRequest`, etc.) | | ||
| | `iii_sdk::stream` | Stream client (`Streams`, `UpdateBuilder`) | |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify where Streams is defined/re-exported and whether iii_sdk::stream exists publicly.
echo "== Find Streams definitions/re-exports =="
rg -n -C2 '\b(pub\s+)?(struct|use)\s+Streams\b|pub\s+mod\s+stream\b|pub\s+mod\s+streams\b'
echo
echo "== Find references to iii_sdk::stream and iii_sdk::streams in docs/code =="
rg -n -C2 'iii_sdk::stream\b|iii_sdk::streams\b|use iii_sdk::\{[^}]*Streams'
echo
echo "== Inspect crate roots likely exporting modules =="
fd 'lib.rs$|mod.rs$|README.md$' | sed -n '1,200p'Repository: iii-hq/iii
Length of output: 5368
🏁 Script executed:
cat -n sdk/packages/rust/iii/src/lib.rsRepository: iii-hq/iii
Length of output: 4572
Fix misleading module path in API reference table.
The README table (line 178) documents the import as iii_sdk::stream, but every code example in the repository imports Streams and UpdateBuilder directly from iii_sdk:
use iii_sdk::{Streams, UpdateBuilder}; // Correct (as per all examples)The module stream is public, and Streams/UpdateBuilder are re-exported at the crate root (lib.rs line 28). Update the table entry to reflect the actual idiomatic import path:
| `iii_sdk` | Stream client (`Streams`, `UpdateBuilder`) |
Alternatively, document both paths if submodule access is intentional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/packages/rust/iii/README.md` at line 178, The README table entry
incorrectly lists the import path as `iii_sdk::stream`; update the table to show
the idiomatic crate-root import `iii_sdk` for the stream client (e.g.,
"`iii_sdk` | Stream client (`Streams`, `UpdateBuilder`)") or, if you intend to
show both, list both paths (e.g., "`iii_sdk` / `iii_sdk::stream` | Stream client
(`Streams`, `UpdateBuilder`)") and ensure the description references the
re-exported symbols `Streams` and `UpdateBuilder` (these are re-exported at the
crate root).
Comment on lines
+39
to
43
| /// ```rust,no_run | ||
| /// use iii_sdk::{register_worker, InitOptions}; | ||
| /// | ||
| /// #[tokio::main] | ||
| /// async fn main() { | ||
| /// let iii = register_worker("ws://localhost:49134", InitOptions::default()).unwrap(); | ||
| /// } | ||
| /// let iii = register_worker("ws://localhost:49134", InitOptions::default()); | ||
| /// ``` |
Contributor
There was a problem hiding this comment.
register_worker() now crashes the caller process on a missing runtime, but the public docs still describe a recoverable error path.
The short example here omits Tokio entirely, and the doc block still says this returns IIIError::Runtime, while the implementation now panics instead. For a library entry point that’s a sharp behavior change — either keep a fallible constructor (Result / try_register_worker) or update the contract to # Panics and make every example run inside a Tokio runtime.
Also applies to: 60-61, 90-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/packages/rust/iii/src/lib.rs` around lines 39 - 43, The docs claim
register_worker returns an IIIError::Runtime on missing Tokio, but the current
implementation panics; restore a fallible API or clearly document the panic. Fix
by making register_worker a fallible constructor (e.g., return Result<...,
IIIError> or add try_register_worker that returns Result and have
register_worker call it) so callers can handle IIIError::Runtime instead of
crashing; update the doc examples and doc comments (the code block showing
register_worker and any references to IIIError::Runtime) to show the Result
return path (or, if you choose to keep the panic, add a clear "# Panics" doc and
ensure examples run inside a Tokio runtime). Ensure you reference and adjust the
register_worker symbol and the IIIError::Runtime variant accordingly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/scripts/parsers/parse-rustdoc.mts (2)
602-612:⚠️ Potential issue | 🟡 MinorMinor inconsistency in fallback entry point return type.
The fallback
entryPointspecifiesreturns: { type: 'Result<III, IIIError>', ... }at line 610, but the signature at line 604 shows-> III(no Result wrapper). This inconsistency could confuse documentation readers.Proposed fix to align the return type
entryPoint: entryPoint ?? { name: 'register_worker', signature: 'register_worker(address: &str, options: InitOptions) -> III', description: 'Create and return a connected SDK instance.', params: [ { name: 'address', type: '&str', description: 'WebSocket URL of the III engine.', required: true }, { name: 'options', type: 'InitOptions', description: 'Configuration for worker metadata and OTel.', required: true }, ], - returns: { type: 'Result<III, IIIError>', description: 'Connected SDK instance.' }, + returns: { type: 'III', description: 'Connected SDK instance.' }, examples: [], },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/scripts/parsers/parse-rustdoc.mts` around lines 602 - 612, The fallback entryPoint object has a mismatch between the signature and returns for register_worker; update the entryPoint.signature string so it matches entryPoint.returns by changing "register_worker(address: &str, options: InitOptions) -> III" to include the Result wrapper (e.g., "register_worker(address: &str, options: InitOptions) -> Result<III, IIIError>") so the signature and returns are consistent; fields to edit are entryPoint.signature and verify entryPoint.returns remain as { type: 'Result<III, IIIError>', ... } for register_worker.
649-654:⚠️ Potential issue | 🟡 MinorSame return type inconsistency in empty doc fallback.
The
createEmptyRustSdkDochas the same issue: signature shows-> IIIbutreturns.typeisResult<III, IIIError>.Proposed fix
entryPoint: { name: 'register_worker', signature: 'register_worker(address: &str, options: InitOptions) -> III', description: 'Create and return a connected SDK instance.', params: [], - returns: { type: 'Result<III, IIIError>', description: '' }, + returns: { type: 'III', description: '' }, examples: [], },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/scripts/parsers/parse-rustdoc.mts` around lines 649 - 654, The empty-doc fallback created by createEmptyRustSdkDoc has a mismatch for the register_worker entry: its signature field currently reads "-> III" while returns.type is "Result<III, IIIError>"; update the register_worker doc object inside createEmptyRustSdkDoc so the signature matches the declared return type (e.g. change signature to "register_worker(address: &str, options: InitOptions) -> Result<III, IIIError>" or, if intended, make returns.type "III") ensuring consistency between signature and returns.type.
🧹 Nitpick comments (1)
docs/scripts/renderers/__tests__/render-mdx.test.mts (1)
75-89: Narrow the negative assertion scope to reduce test brittleness.Line 88 (
expect(mdx).not.toContain('\\{')) checks the entire generated document, so unrelated escaped braces could fail this test later. Prefer asserting against the specific expected sentence/span.Suggested test assertion adjustment
- expect(mdx).not.toContain('\\{') + expect(mdx).toContain('Matches `trigger({ function_id })` signature.') + expect(mdx).not.toContain('Matches `trigger(\\{ function_id \\})` signature.')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/scripts/renderers/__tests__/render-mdx.test.mts` around lines 75 - 89, The test "does not escape braces inside backtick spans in type descriptions" currently asserts globally that the generated mdx does not contain '\\{', which is brittle; locate that test and replace the broad negative assertion (expect(mdx).not.toContain('\\{')) with a scoped assertion that the specific backtick span is present and unescaped — for example, assert that mdx contains the exact substring '`trigger({ function_id })`' (using renderSdkMdx/minimalDoc and the mdx variable already in the test) or extract the matched backtick span via a regex and assert the matched string does not include a backslash, ensuring only the specific sentence/span is checked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/content/api-reference/sdk-rust.mdx`:
- Around line 16-22: The examples calling register_worker(...) lack an active
Tokio runtime and must be wrapped in an async main; for each snippet that calls
register_worker with InitOptions::default() (the first initialization example
and the later snippet around lines 108–111), add #[tokio::main] and async fn
main() { ... } around the call so register_worker runs under a Tokio runtime
(keep the same register_worker and InitOptions identifiers, only wrap the call
in the async main).
---
Outside diff comments:
In `@docs/scripts/parsers/parse-rustdoc.mts`:
- Around line 602-612: The fallback entryPoint object has a mismatch between the
signature and returns for register_worker; update the entryPoint.signature
string so it matches entryPoint.returns by changing "register_worker(address:
&str, options: InitOptions) -> III" to include the Result wrapper (e.g.,
"register_worker(address: &str, options: InitOptions) -> Result<III, IIIError>")
so the signature and returns are consistent; fields to edit are
entryPoint.signature and verify entryPoint.returns remain as { type:
'Result<III, IIIError>', ... } for register_worker.
- Around line 649-654: The empty-doc fallback created by createEmptyRustSdkDoc
has a mismatch for the register_worker entry: its signature field currently
reads "-> III" while returns.type is "Result<III, IIIError>"; update the
register_worker doc object inside createEmptyRustSdkDoc so the signature matches
the declared return type (e.g. change signature to "register_worker(address:
&str, options: InitOptions) -> Result<III, IIIError>" or, if intended, make
returns.type "III") ensuring consistency between signature and returns.type.
---
Nitpick comments:
In `@docs/scripts/renderers/__tests__/render-mdx.test.mts`:
- Around line 75-89: The test "does not escape braces inside backtick spans in
type descriptions" currently asserts globally that the generated mdx does not
contain '\\{', which is brittle; locate that test and replace the broad negative
assertion (expect(mdx).not.toContain('\\{')) with a scoped assertion that the
specific backtick span is present and unescaped — for example, assert that mdx
contains the exact substring '`trigger({ function_id })`' (using
renderSdkMdx/minimalDoc and the mdx variable already in the test) or extract the
matched backtick span via a regex and assert the matched string does not include
a backslash, ensuring only the specific sentence/span is checked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a98e806-8b4b-4198-b64c-15cb5a832caf
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
docs/content/api-reference/sdk-node.mdxdocs/content/api-reference/sdk-rust.mdxdocs/package.jsondocs/scripts/parsers/__tests__/parse-rustdoc.test.mtsdocs/scripts/parsers/parse-rustdoc.mtsdocs/scripts/renderers/__tests__/components.test.mtsdocs/scripts/renderers/__tests__/render-mdx.test.mtsdocs/scripts/renderers/components.mtsdocs/scripts/renderers/render-mdx.mts
✅ Files skipped from review due to trivial changes (1)
- docs/content/api-reference/sdk-node.mdx
Comment on lines
+16
to
22
| The Rust SDK provides `register_worker()` to create a connected SDK instance. The WebSocket connection is established automatically. | ||
|
|
||
| ```rust | ||
| use iii_sdk::{III, InitOptions}; | ||
| use iii_sdk::{register_worker, InitOptions}; | ||
|
|
||
| let iii = III::new("ws://localhost:49134"); | ||
| iii.connect().await?; | ||
| let iii = register_worker("ws://localhost:49134", InitOptions::default()); | ||
| ``` |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify runtime requirement/panic behavior in Rust SDK implementation
rg -n -C2 'pub fn register_worker|try_current|panic!\(|requires an active Tokio runtime' sdk/packages/rust/iii/src/lib.rs
# Verify docs initialization snippet currently lacks explicit Tokio runtime in first example
rg -n -C2 '## Initialization|register_worker\("ws://localhost:49134"|tokio::main|Tokio runtime' docs/content/api-reference/sdk-rust.mdxRepository: iii-hq/iii
Length of output: 1208
Add explicit Tokio runtime requirement to the Initialization section to prevent panics from copied code.
The first initialization example (lines 21) omits the Tokio runtime that register_worker() requires. Users copying this snippet will encounter a panic: "iii_sdk::register_worker requires an active Tokio runtime". Update the first example to include #[tokio::main] and async wrapper, consistent with the later example at lines 47–50.
Suggested patch
## Initialization
The Rust SDK provides `register_worker()` to create a connected SDK instance. The WebSocket connection is established automatically.
+`register_worker()` requires an active Tokio runtime.
```rust
use iii_sdk::{register_worker, InitOptions};
-let iii = register_worker("ws://localhost:49134", InitOptions::default());
+#[tokio::main]
+async fn main() {
+ let iii = register_worker("ws://localhost:49134", InitOptions::default());
+}
</details>
Also apply the same fix to the snippet at lines 108–111.
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
The Rust SDK provides `register_worker()` to create a connected SDK instance. The WebSocket connection is established automatically.
`register_worker()` requires an active Tokio runtime.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/content/api-reference/sdk-rust.mdx` around lines 16 - 22, The examples
calling register_worker(...) lack an active Tokio runtime and must be wrapped in
an async main; for each snippet that calls register_worker with
InitOptions::default() (the first initialization example and the later snippet
around lines 108–111), add #[tokio::main] and async fn main() { ... } around the
call so register_worker runs under a Tokio runtime (keep the same
register_worker and InitOptions identifiers, only wrap the call in the async
main).
…tions and triggers - Replaced direct function and trigger registration calls with structured messages (`RegisterFunctionMessage` and `RegisterTriggerInput`). - Updated the `Cargo.toml` to reflect the new `iii-sdk` dependency structure. - Enhanced error handling in the bridge module and improved OpenTelemetry configuration. - Cleaned up the main application logic to streamline bridge connection and function registration processes. - Updated documentation to reflect changes in function and trigger registration methods.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
sdk/packages/rust/iii/tests/queue_integration.rs (4)
230-281:⚠️ Potential issue | 🟠 MajorMissing
unregister()fortest.queue.multi.rsfunction.Same issue - add cleanup at end of test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/packages/rust/iii/tests/queue_integration.rs` around lines 230 - 281, The test registers a function with iii.register_function using id "test.queue.multi.rs" but never unregisters it; add a cleanup call to unregister that function at the end of the test (after the assertions) by invoking the corresponding unregister method on iii for "test.queue.multi.rs" (awaiting it if it is async) and handling errors (e.g., unwrap/expect) so the function is removed between tests.
289-366:⚠️ Potential issue | 🟠 MajorMissing
unregister()for bothtest.queue.chain.a.rsandtest.queue.chain.b.rsfunctions.Both function registrations in this test need cleanup.
🛠️ Suggested fix
let b_received = Arc::new(Mutex::new(Vec::new())); let b_received_clone = b_received.clone(); - iii.register_function( + let fn_b = iii.register_function( RegisterFunctionMessage { id: "test.queue.chain.b.rs".to_string(), ... let a_received = Arc::new(Mutex::new(Vec::new())); let a_received_clone = a_received.clone(); let iii_for_a = iii.clone(); - iii.register_function( + let fn_a = iii.register_function( RegisterFunctionMessage { id: "test.queue.chain.a.rs".to_string(), ... assert_eq!(b_msgs[0]["from_a"], true); assert_eq!(b_msgs[0]["label"], "chained-work"); + + fn_a.unregister(); + fn_b.unregister(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/packages/rust/iii/tests/queue_integration.rs` around lines 289 - 366, The test registers two functions via iii.register_function ("test.queue.chain.a.rs" and "test.queue.chain.b.rs") but never removes them; add cleanup calls to iii.unregister for both IDs after the assertions (await the calls and handle errors with expect or map_err to fail the test) so the functions are unregistered when the test completes; ensure you call iii.unregister("test.queue.chain.a.rs").await and iii.unregister("test.queue.chain.b.rs").await (or the project's equivalent unregister API) to remove both registrations.
99-144:⚠️ Potential issue | 🟠 MajorMissing
unregister()fortest.queue.fifo.rsfunction.Same issue - this function registration needs cleanup to prevent panics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/packages/rust/iii/tests/queue_integration.rs` around lines 99 - 144, The test registers a function with iii.register_function using id "test.queue.fifo.rs" but never unregisters it, which can cause panics; after the assertions (after checking msgs and amounts) call the corresponding cleanup API (iii.unregister or the test harness unregister() method) for the same function id ("test.queue.fifo.rs") to remove the registration—i.e., locate the iii.register_function(...) block and add a matching iii.unregister("test.queue.fifo.rs").await (or the synchronous unregister call your API exposes) before the test exits so the function is cleaned up.
22-63:⚠️ Potential issue | 🟠 MajorMissing
unregister()call will cause panics on repeated test runs.The function registered with ID
"test.queue.echo.rs"is never unregistered. Sincecommon::shared_iii()returns a static instance that persists across the test binary lifetime, andregister_function_innerpanics on duplicate IDs, this test will panic if:
- Tests run in parallel (cargo's default)
- The test binary is re-run without restarting
Add cleanup similar to
bridge.rstests.🛠️ Suggested fix
- iii.register_function( + let fn_ref = iii.register_function( RegisterFunctionMessage { id: "test.queue.echo.rs".to_string(), ... }, move |input: Value| { ... }, ); ... assert_eq!(msgs[0]["msg"], "hello"); + + fn_ref.unregister(); }Based on learnings from context snippet 1:
register_function_innerpanics if a function ID is already registered, and context snippet 2 shows the shared instance is never cleared between tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/packages/rust/iii/tests/queue_integration.rs` around lines 22 - 63, The test registers a function with id "test.queue.echo.rs" via iii.register_function but never calls unregister(), which causes panics on duplicate registrations across test runs; fix by adding a cleanup call to unregister the function after the test (mirroring bridge.rs) — after the assertions call iii.unregister("test.queue.echo.rs") (or the iii.unregister(...) async variant used elsewhere) and await it so the shared instance from common::shared_iii() is cleared; ensure you reference the same function id string and place the unregister call before the test returns so subsequent runs won't hit register_function_inner duplicate-id panics.
♻️ Duplicate comments (3)
docs/content/api-reference/sdk-rust.mdx (2)
107-124:⚠️ Potential issue | 🟡 MinorAdd Tokio runtime wrapper to
register_functionexample.Same issue as the initialization example - this snippet lacks the required Tokio runtime context.
📝 Suggested fix
```rust use iii_sdk::{register_worker, InitOptions, RegisterFunctionMessage}; use serde_json::{json, Value}; + +#[tokio::main] +async fn main() { let iii = register_worker("ws://localhost:49134", InitOptions::default()); iii.register_function( RegisterFunctionMessage { id: "greet".to_string(), ... }, |input: Value| async move { Ok(json!({"message": format!("Hello, {}!", input["name"])})) }, ); +}</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/content/api-reference/sdk-rust.mdxaround lines 107 - 124, The example
calling register_worker and register_function lacks a Tokio runtime; wrap the
snippet in an async main annotated with #[tokio::main] so register_worker(...,
InitOptions::default()) and the register_function call (with
RegisterFunctionMessage and the async closure) run inside Tokio's runtime; add
an async fn main() { ... } and move the existing calls into it, leaving the
RegisterFunctionMessage fields and the async |input: Value| closure unchanged.</details> --- `18-22`: _⚠️ Potential issue_ | _🟡 Minor_ **Add Tokio runtime wrapper to initialization example.** The first example will panic if copied directly since `register_worker()` requires an active Tokio runtime. Update to match the correct example at lines 47-50. <details> <summary>📝 Suggested fix</summary> ```diff ```rust use iii_sdk::{register_worker, InitOptions}; -let iii = register_worker("ws://localhost:49134", InitOptions::default()); +#[tokio::main] +async fn main() { + let iii = register_worker("ws://localhost:49134", InitOptions::default()); +}</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/content/api-reference/sdk-rust.mdxaround lines 18 - 22, The example
calls register_worker(...) (with InitOptions::default()) outside a Tokio runtime
which will panic; wrap the call in a Tokio runtime entrypoint by creating an
async main annotated with #[tokio::main] and move the register_worker invocation
into that function (and await it if register_worker returns a Future) so the
example shows correct async runtime initialization for register_worker and
InitOptions.</details> </blockquote></details> <details> <summary>sdk/packages/rust/iii/src/lib.rs (1)</summary><blockquote> `59-61`: _⚠️ Potential issue_ | _🟠 Major_ **Documentation claims error return but implementation panics.** The doc comment states "Returns `IIIError::Runtime` if no active Tokio runtime is found" but the implementation at lines 89-94 panics instead. Either: 1. Update docs to document the panic with `# Panics` section, or 2. Change implementation to return `Result<III, IIIError>` <details> <summary>📝 Suggested doc fix (if keeping panic behavior)</summary> ```diff -/// # Errors -/// Returns [`IIIError::Runtime`] if no active Tokio runtime is found. +/// # Panics +/// Panics if no active Tokio runtime is found. Ensure this function is called +/// from within a Tokio runtime context (e.g., inside `#[tokio::main]` or +/// `tokio::runtime::Runtime::block_on`).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/packages/rust/iii/src/lib.rs` around lines 59 - 61, The doc says the constructor returns IIIError::Runtime but the implementation panics when no Tokio runtime is found; change the API to match the doc by updating the constructor (e.g., III::new or fn new in impl III) to return Result<III, IIIError> instead of panicking: replace the panic branch that checks for an active Tokio runtime with returning Err(IIIError::Runtime), update the doc comment to describe the Result return, and update all call sites to handle the Result (propagate with ? or handle Err) so behavior and docs are consistent.
🧹 Nitpick comments (3)
engine/src/modules/stream/adapters/bridge.rs (1)
286-293: Consider logging or handling the trigger registration error.The
register_triggerresult is silently discarded withlet _ = .... While this may be intentional to avoid blocking on non-critical trigger setup, silently ignoring errors could mask configuration issues that prevent event subscriptions from working.🔧 Suggested improvement
- let _ = self.bridge.register_trigger(RegisterTriggerInput { + if let Err(e) = self.bridge.register_trigger(RegisterTriggerInput { trigger_type: "subscribe".to_string(), function_id: handler_function_id, config: serde_json::to_value(SubscribeTrigger { topic: STREAM_EVENTS_TOPIC.to_string(), }) .unwrap_or_default(), - }); + }) { + tracing::warn!(error = %e, "Failed to register stream events subscribe trigger"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/modules/stream/adapters/bridge.rs` around lines 286 - 293, The call to self.bridge.register_trigger(...) currently discards the Result (let _ = ...) and can silently hide failures; change this to capture the Result from bridge.register_trigger(RegisterTriggerInput { ... }) and either propagate the error (return Err) from the surrounding function or log it with context — include the handler_function_id and STREAM_EVENTS_TOPIC and the RegisterTriggerInput/SubscribeTrigger details — so callers/operators can see why subscription registration failed.docs/content/api-reference/sdk-rust.mdx (1)
59-62: Clarify thatshutdown()is synchronous despite spawning a background task.The description mentions spawning a background task for telemetry flush but the signature shows a non-async function. Consider clarifying that the function itself is synchronous and returns immediately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/api-reference/sdk-rust.mdx` around lines 59 - 62, The docs for shutdown() are unclear about sync behavior: clarify that shutdown() is a synchronous function that returns immediately even when the otel feature spawns a background task to flush telemetry, and explicitly contrast it with shutdown_async() which awaits telemetry flush; update the text around the shutdown() and shutdown_async() mentions to state that shutdown() does not wait for the background flush to complete and is non-async (returns immediately), and recommend using shutdown_async() for guaranteed telemetry flush.sdk/packages/rust/iii/tests/api_triggers.rs (1)
60-62: Consider reducing the fixed sleep duration.The combination of
common::settle().awaitplus an additional 500ms sleep appears throughout the file. Ifsettle()already provides synchronization, the extra sleep may be unnecessary or could be reduced to improve test execution time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/packages/rust/iii/tests/api_triggers.rs` around lines 60 - 62, common::settle().await is intended to synchronize the test, so remove or reduce the redundant fixed sleep following it: replace or delete the sleep(Duration::from_millis(500)).await calls in api_triggers.rs (all occurrences) — either remove them entirely if settle() suffices, or reduce to a much smaller value (e.g., 50ms) to speed tests; ensure tests still pass after the change by running the suite.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk/packages/rust/iii/tests/api_triggers.rs`:
- Around line 33-73: The tests register functions and triggers using
iii.register_function and iii.register_trigger but never call unregister,
causing duplicate-registration panics with the shared III instance; update each
test (e.g., api_triggers.rs tests: the GET test and others like
post_endpoint_with_body, path_parameters, query_parameters, custom_status_code,
download_pdf_streaming, upload_pdf_streaming, sse_streaming,
urlencoded_form_data, multipart_form_data) to capture the registration handles
returned by register_function and register_trigger, then call
trigger_handle.unregister() and function_handle.unregister() (or the appropriate
unregister methods on the returned objects) as part of the test teardown (after
assertions or in a finally/cleanup block) to ensure cleanup of registered
entities.
In `@sdk/packages/rust/iii/tests/queue_integration.rs`:
- Around line 185-222: The test registers "test.queue.void.rs" but never
unregisters it; add a cleanup call to unregister that function after the
assertions (e.g. call the iii.unregister(...) or iii.unregister_function(...)
API with id "test.queue.void.rs" and await it), and handle/unwrap the result so
the function is removed once the test completes to avoid leaking registration
across tests.
---
Outside diff comments:
In `@sdk/packages/rust/iii/tests/queue_integration.rs`:
- Around line 230-281: The test registers a function with iii.register_function
using id "test.queue.multi.rs" but never unregisters it; add a cleanup call to
unregister that function at the end of the test (after the assertions) by
invoking the corresponding unregister method on iii for "test.queue.multi.rs"
(awaiting it if it is async) and handling errors (e.g., unwrap/expect) so the
function is removed between tests.
- Around line 289-366: The test registers two functions via
iii.register_function ("test.queue.chain.a.rs" and "test.queue.chain.b.rs") but
never removes them; add cleanup calls to iii.unregister for both IDs after the
assertions (await the calls and handle errors with expect or map_err to fail the
test) so the functions are unregistered when the test completes; ensure you call
iii.unregister("test.queue.chain.a.rs").await and
iii.unregister("test.queue.chain.b.rs").await (or the project's equivalent
unregister API) to remove both registrations.
- Around line 99-144: The test registers a function with iii.register_function
using id "test.queue.fifo.rs" but never unregisters it, which can cause panics;
after the assertions (after checking msgs and amounts) call the corresponding
cleanup API (iii.unregister or the test harness unregister() method) for the
same function id ("test.queue.fifo.rs") to remove the registration—i.e., locate
the iii.register_function(...) block and add a matching
iii.unregister("test.queue.fifo.rs").await (or the synchronous unregister call
your API exposes) before the test exits so the function is cleaned up.
- Around line 22-63: The test registers a function with id "test.queue.echo.rs"
via iii.register_function but never calls unregister(), which causes panics on
duplicate registrations across test runs; fix by adding a cleanup call to
unregister the function after the test (mirroring bridge.rs) — after the
assertions call iii.unregister("test.queue.echo.rs") (or the iii.unregister(...)
async variant used elsewhere) and await it so the shared instance from
common::shared_iii() is cleared; ensure you reference the same function id
string and place the unregister call before the test returns so subsequent runs
won't hit register_function_inner duplicate-id panics.
---
Duplicate comments:
In `@docs/content/api-reference/sdk-rust.mdx`:
- Around line 107-124: The example calling register_worker and register_function
lacks a Tokio runtime; wrap the snippet in an async main annotated with
#[tokio::main] so register_worker(..., InitOptions::default()) and the
register_function call (with RegisterFunctionMessage and the async closure) run
inside Tokio's runtime; add an async fn main() { ... } and move the existing
calls into it, leaving the RegisterFunctionMessage fields and the async |input:
Value| closure unchanged.
- Around line 18-22: The example calls register_worker(...) (with
InitOptions::default()) outside a Tokio runtime which will panic; wrap the call
in a Tokio runtime entrypoint by creating an async main annotated with
#[tokio::main] and move the register_worker invocation into that function (and
await it if register_worker returns a Future) so the example shows correct async
runtime initialization for register_worker and InitOptions.
In `@sdk/packages/rust/iii/src/lib.rs`:
- Around line 59-61: The doc says the constructor returns IIIError::Runtime but
the implementation panics when no Tokio runtime is found; change the API to
match the doc by updating the constructor (e.g., III::new or fn new in impl III)
to return Result<III, IIIError> instead of panicking: replace the panic branch
that checks for an active Tokio runtime with returning Err(IIIError::Runtime),
update the doc comment to describe the Result return, and update all call sites
to handle the Result (propagate with ? or handle Err) so behavior and docs are
consistent.
---
Nitpick comments:
In `@docs/content/api-reference/sdk-rust.mdx`:
- Around line 59-62: The docs for shutdown() are unclear about sync behavior:
clarify that shutdown() is a synchronous function that returns immediately even
when the otel feature spawns a background task to flush telemetry, and
explicitly contrast it with shutdown_async() which awaits telemetry flush;
update the text around the shutdown() and shutdown_async() mentions to state
that shutdown() does not wait for the background flush to complete and is
non-async (returns immediately), and recommend using shutdown_async() for
guaranteed telemetry flush.
In `@engine/src/modules/stream/adapters/bridge.rs`:
- Around line 286-293: The call to self.bridge.register_trigger(...) currently
discards the Result (let _ = ...) and can silently hide failures; change this to
capture the Result from bridge.register_trigger(RegisterTriggerInput { ... })
and either propagate the error (return Err) from the surrounding function or log
it with context — include the handler_function_id and STREAM_EVENTS_TOPIC and
the RegisterTriggerInput/SubscribeTrigger details — so callers/operators can see
why subscription registration failed.
In `@sdk/packages/rust/iii/tests/api_triggers.rs`:
- Around line 60-62: common::settle().await is intended to synchronize the test,
so remove or reduce the redundant fixed sleep following it: replace or delete
the sleep(Duration::from_millis(500)).await calls in api_triggers.rs (all
occurrences) — either remove them entirely if settle() suffices, or reduce to a
much smaller value (e.g., 50ms) to speed tests; ensure tests still pass after
the change by running the suite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f643657e-16ec-49ef-bae1-a2c6b0af72bd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
console/packages/console-rust/Cargo.tomlconsole/packages/console-rust/src/bridge/error.rsconsole/packages/console-rust/src/bridge/functions.rsconsole/packages/console-rust/src/bridge/triggers.rsconsole/packages/console-rust/src/main.rsdocs/content/api-reference/sdk-rust.mdxengine/src/modules/bridge_client/mod.rsengine/src/modules/queue/adapters/bridge.rsengine/src/modules/stream/adapters/bridge.rssdk/packages/rust/iii-example/src/http_example.rssdk/packages/rust/iii-example/src/main.rssdk/packages/rust/iii/src/iii.rssdk/packages/rust/iii/src/lib.rssdk/packages/rust/iii/src/protocol.rssdk/packages/rust/iii/tests/api_triggers.rssdk/packages/rust/iii/tests/bridge.rssdk/packages/rust/iii/tests/data_channels.rssdk/packages/rust/iii/tests/healthcheck.rssdk/packages/rust/iii/tests/http_external_functions.rssdk/packages/rust/iii/tests/init_api.rssdk/packages/rust/iii/tests/pubsub.rssdk/packages/rust/iii/tests/queue_integration.rssdk/packages/rust/iii/tests/state.rs
Comment on lines
+33
to
73
| iii.register_function( | ||
| RegisterFunctionMessage { | ||
| id: "test.api.get.rs".to_string(), | ||
| description: None, | ||
| request_format: None, | ||
| response_format: None, | ||
| metadata: None, | ||
| invocation: None, | ||
| }, | ||
| |_input: Value| async move { | ||
| Ok(json!({ | ||
| "status_code": 200, | ||
| "body": {"message": "Hello from GET"}, | ||
| })) | ||
| }); | ||
|
|
||
| let _trigger = iii | ||
| .register_trigger( | ||
| "http", | ||
| "test.api.get.rs", | ||
| json!({ | ||
| iii | ||
| .register_trigger(RegisterTriggerInput { | ||
| trigger_type: "http".to_string(), | ||
| function_id: "test.api.get.rs".to_string(), | ||
| config: json!({ | ||
| "api_path": "test/rs/hello", | ||
| "http_method": "GET", | ||
| }), | ||
| ) | ||
| }) | ||
| .expect("register trigger"); | ||
|
|
||
| settle().await; | ||
| common::settle().await; | ||
|
|
||
| sleep(Duration::from_millis(500)).await; | ||
|
|
||
| let resp = http_client() | ||
| .get(format!("{}/test/rs/hello", engine_http_url())) | ||
| let resp = common::http_client() | ||
| .get(format!("{}/test/rs/hello", common::engine_http_url())) | ||
| .send() | ||
| .await | ||
| .expect("request failed"); | ||
|
|
||
| assert_eq!(resp.status().as_u16(), 200); | ||
| let data: Value = resp.json().await.expect("json parse"); | ||
| assert_eq!(data["message"], "Hello from GET"); | ||
|
|
||
| iii.shutdown_async().await; | ||
| } |
Contributor
There was a problem hiding this comment.
Missing cleanup for registered functions and triggers throughout this file.
This test and all others in this file (post_endpoint_with_body, path_parameters, query_parameters, custom_status_code, download_pdf_streaming, upload_pdf_streaming, sse_streaming, urlencoded_form_data, multipart_form_data) register functions with hardcoded IDs but never call unregister().
With the shared static III instance from common::shared_iii(), duplicate registrations will panic. Add cleanup similar to:
let fn_ref = iii.register_function(...);
let trigger = iii.register_trigger(...)?;
// ... test logic ...
trigger.unregister();
fn_ref.unregister();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/packages/rust/iii/tests/api_triggers.rs` around lines 33 - 73, The tests
register functions and triggers using iii.register_function and
iii.register_trigger but never call unregister, causing duplicate-registration
panics with the shared III instance; update each test (e.g., api_triggers.rs
tests: the GET test and others like post_endpoint_with_body, path_parameters,
query_parameters, custom_status_code, download_pdf_streaming,
upload_pdf_streaming, sse_streaming, urlencoded_form_data, multipart_form_data)
to capture the registration handles returned by register_function and
register_trigger, then call trigger_handle.unregister() and
function_handle.unregister() (or the appropriate unregister methods on the
returned objects) as part of the test teardown (after assertions or in a
finally/cleanup block) to ensure cleanup of registered entities.
Comment on lines
+185
to
222
| iii.register_function( | ||
| RegisterFunctionMessage { | ||
| id: "test.queue.void.rs".to_string(), | ||
| description: None, | ||
| request_format: None, | ||
| response_format: None, | ||
| metadata: None, | ||
| invocation: None, | ||
| }, | ||
| move |_input: Value| { | ||
| let count = count_clone.clone(); | ||
| async move { | ||
| *count.lock().await += 1; | ||
| Ok(json!({ "done": true })) | ||
| } | ||
| }, | ||
| ); | ||
| common::settle().await; | ||
|
|
||
| let result = iii | ||
| .trigger( | ||
| TriggerRequest::new("test.queue.void.rs", json!({"fire": "forget"})) | ||
| .action(TriggerAction::void()), | ||
| TriggerRequest { | ||
| function_id: "test.queue.void.rs".to_string(), | ||
| payload: json!({"fire": "forget"}), | ||
| action: Some(TriggerAction::Void), | ||
| timeout_ms: None, | ||
| }, | ||
| ) | ||
| .await | ||
| .expect("void should succeed"); | ||
|
|
||
| assert_eq!(result, Value::Null, "void should return null immediately"); | ||
|
|
||
| // Wait for the function to be invoked by the engine | ||
| tokio::time::sleep(Duration::from_secs(2)).await; | ||
|
|
||
| let count = *call_count.lock().await; | ||
| assert_eq!(count, 1, "function should have been called exactly once"); | ||
|
|
||
| iii.shutdown_async().await; | ||
| } |
Contributor
There was a problem hiding this comment.
Missing unregister() for test.queue.void.rs function.
Same issue - add cleanup at end of test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/packages/rust/iii/tests/queue_integration.rs` around lines 185 - 222, The
test registers "test.queue.void.rs" but never unregisters it; add a cleanup call
to unregister that function after the assertions (e.g. call the
iii.unregister(...) or iii.unregister_function(...) API with id
"test.queue.void.rs" and await it), and handle/unwrap the result so the function
is removed once the test completes to avoid leaking registration across tests.
- Updated function registration syntax for better readability in `functions.rs`, `bridge.rs`, and `state.rs`. - Enhanced error handling in `state.rs` by simplifying error inspection. - Cleaned up imports in various files for consistency and clarity. - Reformatted code blocks for improved alignment and structure in `http_example.rs` and test files. - Adjusted comments and spacing for better code maintainability.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
engine/src/modules/queue/adapters/bridge.rs (1)
182-267:⚠️ Potential issue | 🟠 MajorSubscription lifecycle leaks registered functions.
register_function()(line 182) returns aFunctionRefthat enables cleanup viaunregister(), but the return value is dropped without being stored. Functions accumulate indefinitely because only the trigger is cleaned up on unsubscribe, leaving registered function handlers with no way to unregister.The fix requires:
- Import
FunctionReffromiii_sdk- Add
function_ref: FunctionReffield toSubscriptionInfo- Capture the return value from
register_function()and store it- Call
function_ref.unregister()in the unsubscribe handler (in addition to trigger cleanup)- Handle the error path where
register_trigger()fails—unregister the function there tooSuggested fix
use iii_sdk::{ - III, IIIError, InitOptions, RegisterFunctionMessage, RegisterTriggerInput, Trigger, + FunctionRef, III, IIIError, InitOptions, RegisterFunctionMessage, RegisterTriggerInput, Trigger, TriggerAction, TriggerRequest, register_worker, }; struct SubscriptionInfo { trigger: Trigger, + function_ref: FunctionRef, } @@ - self.bridge.register_function( + let function_ref = self.bridge.register_function( RegisterFunctionMessage { id: handler_path.clone(), @@ - let trigger = match self.bridge.register_trigger(RegisterTriggerInput { + let trigger = match self.bridge.register_trigger(RegisterTriggerInput { trigger_type: "queue".to_string(), function_id: handler_path.clone(), config: serde_json::json!({ "topic": topic }), }) { Ok(t) => t, Err(e) => { + function_ref.unregister(); tracing::error!( @@ - subs.insert(key, SubscriptionInfo { trigger }); + subs.insert( + key, + SubscriptionInfo { + trigger, + function_ref, + }, + ); @@ if let Some(subscription) = subs.remove(&key) { subscription.trigger.unregister(); + subscription.function_ref.unregister(); - // Note: Bridge doesn't have unregister_function, functions persist - // for bridge lifetime, which is acceptable since bridge lives with adapter }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/modules/queue/adapters/bridge.rs` around lines 182 - 267, The registered bridge handler is leaked because register_function(...)'s returned FunctionRef is dropped; update the code to import iii_sdk::FunctionRef, add a function_ref: FunctionRef field to SubscriptionInfo, capture the FunctionRef returned by self.bridge.register_function(...) and store it in the SubscriptionInfo when creating the subscription, call function_ref.unregister() in the unsubscribe path (alongside existing trigger cleanup), and ensure that if register_trigger(...) fails you call function_ref.unregister() to avoid leaving the function registered. Use the symbols register_function, FunctionRef, SubscriptionInfo, function_ref.unregister, and register_trigger to locate and implement these changes.
♻️ Duplicate comments (5)
sdk/packages/rust/iii/src/iii.rs (1)
524-531:⚠️ Potential issue | 🟡 MinorAlign docs with the supported initialization path (
register_worker).Line 524-Line 531 still shows
III::new(...)for trigger registration, butconnect()is crate-private. This snippet suggests a path that external users cannot fully activate.Suggested doc fix
- /// # use iii_sdk::{III, RegisterTriggerInput}; + /// # use iii_sdk::{register_worker, InitOptions, RegisterTriggerInput}; /// # use serde_json::json; - /// # let iii = III::new("ws://localhost:49134"); + /// # let iii = register_worker("ws://localhost:49134", InitOptions::default());#!/bin/bash # Verify connect visibility and the current docs snippet. rg -n "pub\\(crate\\) fn connect|pub fn new\\(|# use iii_sdk::\\{III, RegisterTriggerInput\\}|III::new\\(\"ws://localhost:49134\"\\)" sdk/packages/rust/iii/src/iii.rs -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/packages/rust/iii/src/iii.rs` around lines 524 - 531, The doc example shows creating an III instance via III::new(...) and then registering a trigger, but connect() is crate-private; update the snippet to use the supported public initialization path by calling register_worker (or the public constructor/wrapper that calls connect) before register_trigger. Specifically, replace references to III::new(...) with the public flow that exposes register_worker (or the SDK's public factory function), keeping the rest of the example (register_trigger and RegisterTriggerInput) intact and remove or comment any usage of the crate-private connect() or III::new symbols so the example compiles for external users.sdk/packages/rust/iii/tests/queue_integration.rs (1)
22-39:⚠️ Potential issue | 🟠 MajorShared singleton tests need explicit unregister cleanup.
With
common::shared_iii()and fixed IDs, registrations persist across tests. Without cleanup, subsequent tests can fail with duplicate registration or become order-dependent.#!/bin/bash # Verify singleton lifecycle and missing per-test cleanup. rg -n "shared_iii\\(|register_function\\(|\\.unregister\\(" sdk/packages/rust/iii/tests/common/mod.rs sdk/packages/rust/iii/tests/queue_integration.rs -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/packages/rust/iii/tests/queue_integration.rs` around lines 22 - 39, The test registers a function with iii.register_function using the fixed id "test.queue.echo.rs" against the shared singleton (common::shared_iii()), so you must explicitly unregister that id to avoid cross-test pollution; after the async handler finishes (before/after common::settle().await) call the instance's unregister method for "test.queue.echo.rs" (or wrap the registration in a scope/finally/Drop-style cleanup) so the registration created by RegisterFunctionMessage is removed and subsequent tests won't see a duplicate registration.sdk/packages/rust/iii/tests/api_triggers.rs (1)
33-60:⚠️ Potential issue | 🟠 MajorAdd teardown cleanup for registered functions/triggers.
Using
common::shared_iii()with hardcoded IDs and no unregister leads to test interference and duplicate-registration failures over time.#!/bin/bash # Verify function/trigger registrations and cleanup calls in this test file. rg -n "shared_iii\\(|register_function\\(|register_trigger\\(|\\.unregister\\(" sdk/packages/rust/iii/tests/api_triggers.rs sdk/packages/rust/iii/tests/common/mod.rs -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/packages/rust/iii/tests/api_triggers.rs` around lines 33 - 60, The test registers a function ("test.api.get.rs") and a trigger (RegisterTriggerInput with api_path "test/rs/hello") against common::shared_iii() but never unregisters them, causing duplicate-registration failures; fix by capturing the registration results from iii.register_function and iii.register_trigger (or generating unique IDs instead of hardcoding "test.api.get.rs"), then call the corresponding unregister APIs (e.g. iii.unregister_function(function_id) and iii.unregister_trigger(trigger_id) or iii.unregister(...) as provided by the III client) in the test teardown (after common::settle().await or via a drop/cleanup block), ensuring both the function_id ("test.api.get.rs") and the created trigger id are removed.sdk/packages/rust/iii/src/stream.rs (1)
8-10:⚠️ Potential issue | 🟡 MinorFix doctest example:
register_workershould not use?.Line 10 still uses
?onregister_worker(...), which is inconsistent with the current API and produces a misleading snippet.#!/bin/bash # Verify current API signature and the doc example. rg -n "pub fn register_worker\\(" sdk/packages/rust/iii/src/lib.rs -C2 rg -n "register_worker\\(\"ws://localhost:49134\", InitOptions::default\\)\\?" sdk/packages/rust/iii/src/stream.rs -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/packages/rust/iii/src/stream.rs` around lines 8 - 10, The doctest in stream.rs incorrectly uses the `?` operator on `register_worker(...)`; update the example to call `register_worker("ws://localhost:49134", InitOptions::default())` without the `?` (e.g., assign the Result directly or call .expect/.unwrap if you want to show panicking behavior) so the snippet matches the current `register_worker` API; change the line in the doctest that contains `register_worker(...)?` to remove the `?` and ensure variable `iii` is assigned from `register_worker` accordingly.engine/src/modules/queue/adapters/bridge.rs (1)
71-78:⚠️ Potential issue | 🟠 Major
BridgeAdapter::newerror contract no longer matches implementation.Line 73 says connection failures are returned, but Line 77 uses
register_worker(...)and then always returnsOk(Self { ... }). Update docs or make construction actually validate connectivity.#!/bin/bash # Verify constructor docs/behavior vs register_worker signature. rg -n "Returns error if bridge connection fails|pub async fn new\\(" engine/src/modules/queue/adapters/bridge.rs -C2 rg -n "pub fn register_worker\\(" sdk/packages/rust/iii/src/lib.rs -C2
🧹 Nitpick comments (1)
console/packages/console-rust/src/bridge/functions.rs (1)
35-48: Consider a small helper to centralize trigger defaults.There is substantial duplicated trigger boilerplate (
action: None, timeout wiring,function_idconversion). A helper would reduce drift and make timeout policy changes safer.Refactor sketch
+async fn trigger_json( + bridge: &III, + function_id: &str, + payload: Value, + timeout_ms: u64, +) -> Result<Value, iii_sdk::IIIError> { + bridge + .trigger(TriggerRequest { + function_id: function_id.to_string(), + payload, + action: None, + timeout_ms: Some(timeout_ms), + }) + .await +}Also applies to: 50-63, 65-97, 99-147, 149-198, 200-340, 342-387, 389-550, 552-758, 760-1014
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/packages/console-rust/src/bridge/functions.rs` around lines 35 - 48, The trigger calls (e.g., in handle_health) repeat boilerplate (action: None, timeout_ms, function_id string conversion); add a small helper function (e.g., make_trigger_request or trigger_with_defaults) that accepts function_id: &str, payload: serde_json::Value and optional action/timeout parameters and returns a TriggerRequest populated with the common defaults, then replace direct TriggerRequest constructions in handle_health and all handlers that call bridge.trigger(...) with calls to this helper to centralize timeout and action defaults and remove duplicated conversion of function_id to String.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/src/modules/state/adapters/bridge.rs`:
- Line 59: The code currently silences serde_json::to_value serialization
failures by using unwrap_or(Value::Null) when building the payload in the state
bridge operations (set, get, update, delete, list, list_groups); instead
return/propagate the serialization error. Replace occurrences like payload:
serde_json::to_value(data).unwrap_or(serde_json::Value::Null) with a fallible
conversion (e.g. let payload = serde_json::to_value(data)? or map the serde
error into the function's error type and return it) so that functions such as
set, get, update, delete, list, and list_groups fail on serialization errors
rather than sending Null payloads.
In `@sdk/packages/rust/iii-example/src/http_example.rs`:
- Around line 84-85: The current parsing of `input` into `req: ApiRequest` uses
`unwrap_or_else(... .unwrap())`, which can panic on malformed input; replace
this fallback with safe error handling: attempt `serde_json::from_value(input)`
and on Err either construct a safe default `ApiRequest` (e.g.,
`ApiRequest::default()` or a validated empty request) or return a controlled
error response instead of calling `unwrap()`. Update the code around the `req:
ApiRequest` assignment so it does not call `.unwrap()` in the error path and
uses explicit Result handling (match/if let) to log the parse error and handle
it gracefully.
In `@sdk/packages/rust/iii/src/stream.rs`:
- Around line 96-101: The code silently converts serialization failures to
serde_json::Value::Null by using serde_json::to_value(input).unwrap_or(...);
change this to surface the error instead: call serde_json::to_value(input) and
handle the Result (e.g., propagate the error from the current function, return a
Result with a descriptive error, or map the error into a TriggerRequest
construction failure) rather than unwrapping to Null; update the code around
TriggerRequest creation (where serde_json::to_value(input) is used) so
serialization errors are returned or logged with context instead of replaced
with Null.
---
Outside diff comments:
In `@engine/src/modules/queue/adapters/bridge.rs`:
- Around line 182-267: The registered bridge handler is leaked because
register_function(...)'s returned FunctionRef is dropped; update the code to
import iii_sdk::FunctionRef, add a function_ref: FunctionRef field to
SubscriptionInfo, capture the FunctionRef returned by
self.bridge.register_function(...) and store it in the SubscriptionInfo when
creating the subscription, call function_ref.unregister() in the unsubscribe
path (alongside existing trigger cleanup), and ensure that if
register_trigger(...) fails you call function_ref.unregister() to avoid leaving
the function registered. Use the symbols register_function, FunctionRef,
SubscriptionInfo, function_ref.unregister, and register_trigger to locate and
implement these changes.
---
Duplicate comments:
In `@sdk/packages/rust/iii/src/iii.rs`:
- Around line 524-531: The doc example shows creating an III instance via
III::new(...) and then registering a trigger, but connect() is crate-private;
update the snippet to use the supported public initialization path by calling
register_worker (or the public constructor/wrapper that calls connect) before
register_trigger. Specifically, replace references to III::new(...) with the
public flow that exposes register_worker (or the SDK's public factory function),
keeping the rest of the example (register_trigger and RegisterTriggerInput)
intact and remove or comment any usage of the crate-private connect() or
III::new symbols so the example compiles for external users.
In `@sdk/packages/rust/iii/src/stream.rs`:
- Around line 8-10: The doctest in stream.rs incorrectly uses the `?` operator
on `register_worker(...)`; update the example to call
`register_worker("ws://localhost:49134", InitOptions::default())` without the
`?` (e.g., assign the Result directly or call .expect/.unwrap if you want to
show panicking behavior) so the snippet matches the current `register_worker`
API; change the line in the doctest that contains `register_worker(...)?` to
remove the `?` and ensure variable `iii` is assigned from `register_worker`
accordingly.
In `@sdk/packages/rust/iii/tests/api_triggers.rs`:
- Around line 33-60: The test registers a function ("test.api.get.rs") and a
trigger (RegisterTriggerInput with api_path "test/rs/hello") against
common::shared_iii() but never unregisters them, causing duplicate-registration
failures; fix by capturing the registration results from iii.register_function
and iii.register_trigger (or generating unique IDs instead of hardcoding
"test.api.get.rs"), then call the corresponding unregister APIs (e.g.
iii.unregister_function(function_id) and iii.unregister_trigger(trigger_id) or
iii.unregister(...) as provided by the III client) in the test teardown (after
common::settle().await or via a drop/cleanup block), ensuring both the
function_id ("test.api.get.rs") and the created trigger id are removed.
In `@sdk/packages/rust/iii/tests/queue_integration.rs`:
- Around line 22-39: The test registers a function with iii.register_function
using the fixed id "test.queue.echo.rs" against the shared singleton
(common::shared_iii()), so you must explicitly unregister that id to avoid
cross-test pollution; after the async handler finishes (before/after
common::settle().await) call the instance's unregister method for
"test.queue.echo.rs" (or wrap the registration in a scope/finally/Drop-style
cleanup) so the registration created by RegisterFunctionMessage is removed and
subsequent tests won't see a duplicate registration.
---
Nitpick comments:
In `@console/packages/console-rust/src/bridge/functions.rs`:
- Around line 35-48: The trigger calls (e.g., in handle_health) repeat
boilerplate (action: None, timeout_ms, function_id string conversion); add a
small helper function (e.g., make_trigger_request or trigger_with_defaults) that
accepts function_id: &str, payload: serde_json::Value and optional
action/timeout parameters and returns a TriggerRequest populated with the common
defaults, then replace direct TriggerRequest constructions in handle_health and
all handlers that call bridge.trigger(...) with calls to this helper to
centralize timeout and action defaults and remove duplicated conversion of
function_id to String.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef4ddd78-69b6-4d3b-b28a-c52842625ab9
📒 Files selected for processing (12)
cli/src/state.rsconsole/packages/console-rust/src/bridge/functions.rsconsole/packages/console-rust/src/bridge/triggers.rsengine/src/modules/observability/mod.rsengine/src/modules/queue/adapters/bridge.rsengine/src/modules/state/adapters/bridge.rssdk/packages/rust/iii-example/src/http_example.rssdk/packages/rust/iii/src/iii.rssdk/packages/rust/iii/src/logger.rssdk/packages/rust/iii/src/stream.rssdk/packages/rust/iii/tests/api_triggers.rssdk/packages/rust/iii/tests/queue_integration.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- engine/src/modules/observability/mod.rs
| .trigger(iii_sdk::TriggerRequest::new("state::update", data)) | ||
| .trigger(TriggerRequest { | ||
| function_id: "state::update".to_string(), | ||
| payload: serde_json::to_value(data).unwrap_or(serde_json::Value::Null), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all silent null-fallback payload serializations in this adapter.
rg -n 'serde_json::to_value\(.+\)\.unwrap_or\(serde_json::Value::Null\)' engine/src/modules/state/adapters/bridge.rsRepository: iii-hq/iii
Length of output: 511
🏁 Script executed:
# Check the total lines in the file and examine context around lines 158-159
wc -l engine/src/modules/state/adapters/bridge.rs
# Read the file with context around the mentioned lines
sed -n '50,70p' engine/src/modules/state/adapters/bridge.rs
sed -n '80,95p' engine/src/modules/state/adapters/bridge.rs
sed -n '100,115p' engine/src/modules/state/adapters/bridge.rs
sed -n '120,135p' engine/src/modules/state/adapters/bridge.rs
sed -n '140,160p' engine/src/modules/state/adapters/bridge.rs
sed -n '150,170p' engine/src/modules/state/adapters/bridge.rsRepository: iii-hq/iii
Length of output: 4282
Don't silently coerce serialization failures to null payloads.
unwrap_or(serde_json::Value::Null) hides serialization errors and may send invalid payloads to mutating endpoints across all state bridge operations (set, get, update, delete, list, list_groups). Propagate serialization errors instead.
Affected lines: 59, 85, 105, 124, 142, 158-159
Suggested patch
+fn to_payload<T: serde::Serialize>(value: T) -> anyhow::Result<serde_json::Value> {
+ serde_json::to_value(value)
+ .map_err(|e| anyhow::anyhow!("Failed to serialize bridge payload: {}", e))
+}
+
.trigger(TriggerRequest {
function_id: "state::set".to_string(),
- payload: serde_json::to_value(data).unwrap_or(serde_json::Value::Null),
+ payload: to_payload(data)?,
action: None,
timeout_ms: None,
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/src/modules/state/adapters/bridge.rs` at line 59, The code currently
silences serde_json::to_value serialization failures by using
unwrap_or(Value::Null) when building the payload in the state bridge operations
(set, get, update, delete, list, list_groups); instead return/propagate the
serialization error. Replace occurrences like payload:
serde_json::to_value(data).unwrap_or(serde_json::Value::Null) with a fallible
conversion (e.g. let payload = serde_json::to_value(data)? or map the serde
error into the function's error type and return it) so that functions such as
set, get, update, delete, list, and list_groups fail on serialization errors
rather than sending Null payloads.
Comment on lines
+84
to
+85
| let req: ApiRequest = serde_json::from_value(input) | ||
| .unwrap_or_else(|_| serde_json::from_value(json!({})).unwrap()); |
Contributor
There was a problem hiding this comment.
Avoid panic in request parsing fallback.
Line 85 uses unwrap() in the error fallback path; malformed input can panic the handler.
Suggested fix
- let req: ApiRequest = serde_json::from_value(input)
- .unwrap_or_else(|_| serde_json::from_value(json!({})).unwrap());
+ let req: ApiRequest = serde_json::from_value(input)
+ .map_err(|e| IIIError::Handler(format!("invalid ApiRequest payload: {e}")))?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/packages/rust/iii-example/src/http_example.rs` around lines 84 - 85, The
current parsing of `input` into `req: ApiRequest` uses `unwrap_or_else(...
.unwrap())`, which can panic on malformed input; replace this fallback with safe
error handling: attempt `serde_json::from_value(input)` and on Err either
construct a safe default `ApiRequest` (e.g., `ApiRequest::default()` or a
validated empty request) or return a controlled error response instead of
calling `unwrap()`. Update the code around the `req: ApiRequest` assignment so
it does not call `.unwrap()` in the error path and uses explicit Result handling
(match/if let) to log the parse error and handle it gracefully.
Comment on lines
+96
to
+101
| .trigger(TriggerRequest { | ||
| function_id: "stream::update".to_string(), | ||
| payload: serde_json::to_value(input).unwrap_or(serde_json::Value::Null), | ||
| action: None, | ||
| timeout_ms: None, | ||
| }) |
Contributor
There was a problem hiding this comment.
Don’t silently replace serialization failure with Null.
Line 98 hides serialization errors and sends a Null payload, which obscures the real failure and can trigger confusing downstream behavior.
Suggested fix
- let result = self
- .iii
- .trigger(TriggerRequest {
- function_id: "stream::update".to_string(),
- payload: serde_json::to_value(input).unwrap_or(serde_json::Value::Null),
- action: None,
- timeout_ms: None,
- })
+ let payload = serde_json::to_value(input).map_err(|e| IIIError::Serde(e.to_string()))?;
+ let result = self
+ .iii
+ .trigger(TriggerRequest {
+ function_id: "stream::update".to_string(),
+ payload,
+ action: None,
+ timeout_ms: None,
+ })
.await?;📝 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
| .trigger(TriggerRequest { | |
| function_id: "stream::update".to_string(), | |
| payload: serde_json::to_value(input).unwrap_or(serde_json::Value::Null), | |
| action: None, | |
| timeout_ms: None, | |
| }) | |
| let payload = serde_json::to_value(input).map_err(|e| IIIError::Serde(e.to_string()))?; | |
| let result = self | |
| .iii | |
| .trigger(TriggerRequest { | |
| function_id: "stream::update".to_string(), | |
| payload, | |
| action: None, | |
| timeout_ms: None, | |
| }) | |
| .await?; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/packages/rust/iii/src/stream.rs` around lines 96 - 101, The code silently
converts serialization failures to serde_json::Value::Null by using
serde_json::to_value(input).unwrap_or(...); change this to surface the error
instead: call serde_json::to_value(input) and handle the Result (e.g., propagate
the error from the current function, return a Result with a descriptive error,
or map the error into a TriggerRequest construction failure) rather than
unwrapping to Null; update the code around TriggerRequest creation (where
serde_json::to_value(input) is used) so serialization errors are returned or
logged with context instead of replaced with Null.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Breaking Changes
Refactor
Tests
Documentation