refactor(rust-sdk)!: simplify register_function — 3 constructors + IIIError handlers#1578
Open
guibeira wants to merge 17 commits into
Open
refactor(rust-sdk)!: simplify register_function — 3 constructors + IIIError handlers#1578guibeira wants to merge 17 commits into
guibeira wants to merge 17 commits into
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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:
📝 WalkthroughWalkthroughMigrate Rust function registration to an id-first API: ChangesRegister API core
Callsites, bridges, and worker modules
Docs, changelog, and doc-generation
Tests
Sequence Diagram(s)sequenceDiagram
participant Worker
participant III_SDK as "III (SDK)"
participant Engine
participant Remote as "Remote/Invoker"
rect rgba(200,230,201,0.5)
Worker->>III_SDK: iii.register_function("id", RegisterFunction::new_async(handler))
III_SDK->>Engine: send RegisterFunctionMessage(id, registration)
end
rect rgba(187,222,251,0.5)
Remote->>Engine: invoke function(id, payload)
Engine->>III_SDK: route invocation to registered handler
III_SDK->>Worker: call handler(payload)
Worker-->>III_SDK: Result<Value, IIIError>
III_SDK-->>Engine: map to invocation response
Engine-->>Remote: return response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![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: 1
🤖 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/changelog/0-11-0/align-rust-register-function-with-signature.mdx`:
- Line 13: Update the table entry for the Python API so the signature for
register_function matches the real API: rename the second parameter from handler
to handler_or_invocation and remove the keyword-only marker "*" so it reads
register_function(function_id, handler_or_invocation, description=None,
metadata=None, request_format=None, response_format=None) (locate the Python
signature line for register_function in the changelog file to make this edit).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f3a1a312-be07-4968-b461-2865796d5ece
📒 Files selected for processing (12)
console/packages/console-rust/src/bridge/functions.rsdocs/api-reference/sdk-python.mdxdocs/api-reference/sdk-rust.mdxdocs/changelog/0-11-0/align-rust-register-function-with-signature.mdxdocs/scripts/generate-api-docs.mtsdocs/scripts/parsers/parse-rustdoc.mtssdk/packages/rust/iii/src/iii.rssdk/packages/rust/iii/src/lib.rssdk/packages/rust/iii/src/protocol.rssdk/packages/rust/iii/tests/init_api.rsskills/iii-rust-sdk/SKILL.mdskills/references/http-invoked-functions.rs
| | SDK | Full-options signature | | ||
| | --- | --- | | ||
| | Node | `registerFunction(functionId, handler, options?)` | | ||
| | Python | `register_function(function_id, handler, *, description, metadata, request_format, response_format)` | |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify signature naming consistency between changelog and Python API reference
rg -n "register_function\\(" docs/api-reference/sdk-python.mdx docs/changelog/0-11-0/align-rust-register-function-with-signature.mdx -C 2Repository: iii-hq/iii
Length of output: 4347
Use Python's canonical handler_or_invocation parameter name and remove the keyword-only marker.
The Python signature in the comparison table should match the actual API documentation: the parameter is named handler_or_invocation (not handler), and since these optional parameters accept positional arguments with defaults, the * keyword-only marker is incorrect.
✏️ Suggested edit
-| Python | `register_function(function_id, handler, *, description, metadata, request_format, response_format)` |
+| Python | `register_function(function_id, handler_or_invocation, description, metadata, request_format, response_format)` |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/changelog/0-11-0/align-rust-register-function-with-signature.mdx` at
line 13, Update the table entry for the Python API so the signature for
register_function matches the real API: rename the second parameter from handler
to handler_or_invocation and remove the keyword-only marker "*" so it reads
register_function(function_id, handler_or_invocation, description=None,
metadata=None, request_format=None, response_format=None) (locate the Python
signature line for register_function in the changelog file to make this edit).
guibeira
changed the title
guibeira
changed the title
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/workers/queue/adapters/bridge.rs (1)
184-255:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStrip bridge trace metadata before invoking the condition and target handler.
This closure reads
__traceparent/__baggageand then forwardsdataunchanged intocheck_condition(...)andengine.call(...). That leaks the internal bridge envelope into user payloads and contradicts the contract documented inbuild_enqueue_payload().Suggested fix
RegisterFunction::new_async(move |data: Value| { let engine = Arc::clone(&engine); let function_id = function_id_owned.clone(); let condition_function_id = condition_function_id_owned.clone(); let topic_name = topic_owned.clone(); async move { - // Extract trace context embedded by the sender - let traceparent = data["__traceparent"].as_str().map(|s| s.to_string()); - let baggage = data["__baggage"].as_str().map(|s| s.to_string()); + // Extract trace context embedded by the sender + let mut payload = data; + let traceparent = payload + .get("__traceparent") + .and_then(Value::as_str) + .map(str::to_owned); + let baggage = payload + .get("__baggage") + .and_then(Value::as_str) + .map(str::to_owned); + if let Some(obj) = payload.as_object_mut() { + obj.remove("__traceparent"); + obj.remove("__baggage"); + } let span = tracing::info_span!( "queue_job", @@ - match check_condition(engine.as_ref(), &condition_path, data.clone()) + match check_condition(engine.as_ref(), &condition_path, payload.clone()) .await { @@ - match engine.call(&function_id, data).await { + match engine.call(&function_id, payload).await {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/workers/queue/adapters/bridge.rs` around lines 184 - 255, The closure registered with iii_sdk::RegisterFunction::new_async reads __traceparent and __baggage but passes the original data through to check_condition(...) and engine.call(...), leaking bridge envelope fields; fix by removing those keys from the payload before invoking check_condition and engine.call (e.g., create a cleaned Value without "__traceparent" and "__baggage" and use that cleaned value for check_condition(engine.as_ref(), &condition_path, cleaned.clone()) and engine.call(&function_id, cleaned).await), keeping the extracted traceparent/baggage for span creation only and referencing the RegisterFunction closure, check_condition, engine.call, and build_enqueue_payload in your changes.
🤖 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/changelog/0-11-0/align-rust-register-function-with-signature.mdx`:
- Around line 78-80: The migration table incorrectly points sync handler usages
to RegisterFunction::new_async(...) — update the rows for synchronous
replacements so they reference RegisterFunction::new(...) instead of new_async;
specifically change the replacements for III::register_function_with(id,
handler, options), the tuple form register_function((RegisterFunctionMessage,
handler)), and RegisterFunction::untyped(f) so that sync handler variants map to
RegisterFunction::new(...) while keeping async variants mapped to
RegisterFunction::new_async(...); apply the same correction for the other
occurrences noted around lines 105-110 to keep argument shapes consistent across
SDKs.
In `@sdk/packages/rust/iii/src/iii.rs`:
- Around line 765-773: The docs for register_function erroneously list
RegisterFunction::new_async twice; update the doc comment for the
register_function item so the constructor list reads the three actual
constructors: RegisterFunction::new, RegisterFunction::new_async, and
RegisterFunction::http (remove the duplicated RegisterFunction::new_async
entry), and verify the RegisterFunction doc string and any cross-SDK references
use those exact names for consistency.
In `@sdk/packages/rust/iii/tests/queue_integration.rs`:
- Around line 356-357: The test registers functions using dot-separated IDs
derived from function_id and RegisterFunction::new_async; update all such base
ID strings (e.g., "test.queue...") to use double-colon separators
("test::queue::...") so durable test function IDs follow the repo standard;
locate occurrences around RegisterFunction::new_async and any places where
function_id.clone() or explicit string IDs are built and replace dots with "::"
consistently (also update the other noted instances where
RegisterFunction::new_async is used).
---
Outside diff comments:
In `@engine/src/workers/queue/adapters/bridge.rs`:
- Around line 184-255: The closure registered with
iii_sdk::RegisterFunction::new_async reads __traceparent and __baggage but
passes the original data through to check_condition(...) and engine.call(...),
leaking bridge envelope fields; fix by removing those keys from the payload
before invoking check_condition and engine.call (e.g., create a cleaned Value
without "__traceparent" and "__baggage" and use that cleaned value for
check_condition(engine.as_ref(), &condition_path, cleaned.clone()) and
engine.call(&function_id, cleaned).await), keeping the extracted
traceparent/baggage for span creation only and referencing the RegisterFunction
closure, check_condition, engine.call, and build_enqueue_payload in your
changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3a3ccc2f-3e22-40da-98c6-5be9bb31c821
📒 Files selected for processing (42)
console/packages/console-rust/src/bridge/functions.rsconsole/packages/console-rust/src/main.rscrates/iii-worker/src/sandbox_daemon/fs/chmod.rscrates/iii-worker/src/sandbox_daemon/fs/grep.rscrates/iii-worker/src/sandbox_daemon/fs/ls.rscrates/iii-worker/src/sandbox_daemon/fs/mkdir.rscrates/iii-worker/src/sandbox_daemon/fs/mv.rscrates/iii-worker/src/sandbox_daemon/fs/read.rscrates/iii-worker/src/sandbox_daemon/fs/rm.rscrates/iii-worker/src/sandbox_daemon/fs/sed.rscrates/iii-worker/src/sandbox_daemon/fs/stat.rscrates/iii-worker/src/sandbox_daemon/fs/write.rscrates/iii-worker/src/sandbox_daemon/mod.rsdocs/api-reference/sdk-rust.mdxdocs/changelog/0-11-0/align-rust-register-function-with-signature.mdxengine/src/workers/bridge_client/mod.rsengine/src/workers/queue/adapters/bridge.rsengine/src/workers/stream/adapters/bridge.rssdk/packages/rust/iii-example/src/cron_trigger_example.rssdk/packages/rust/iii-example/src/custom_trigger_example.rssdk/packages/rust/iii-example/src/http_example.rssdk/packages/rust/iii-example/src/logger_example.rssdk/packages/rust/iii-example/src/main.rssdk/packages/rust/iii-example/src/trigger_type_example.rssdk/packages/rust/iii/src/error.rssdk/packages/rust/iii/src/iii.rssdk/packages/rust/iii/src/lib.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/iii_fn_test.rssdk/packages/rust/iii/tests/init_api.rssdk/packages/rust/iii/tests/middleware.rssdk/packages/rust/iii/tests/pubsub.rssdk/packages/rust/iii/tests/queue_integration.rssdk/packages/rust/iii/tests/rbac_workers.rssdk/packages/rust/iii/tests/register_function_test.rssdk/packages/rust/iii/tests/state.rsskills/iii-rust-sdk/SKILL.mdskills/references/http-invoked-functions.rs
💤 Files with no reviewable changes (3)
- console/packages/console-rust/src/main.rs
- sdk/packages/rust/iii/tests/iii_fn_test.rs
- sdk/packages/rust/iii/tests/register_function_test.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- sdk/packages/rust/iii/tests/init_api.rs
- skills/iii-rust-sdk/SKILL.md
- console/packages/console-rust/src/bridge/functions.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/src/iii.rs`:
- Around line 372-376: The code currently converts serde_json errors from
serde_json::from_value and serde_json::to_value into IIIError::Handler, losing
the distinction between user handler failures and serialization errors; update
the map_err closures around serde_json::from_value::<T>(input) and
serde_json::to_value(&val) to return IIIError::Serde(e.to_string()) instead of
IIIError::Handler, and make the same change in the other adapter branch that
handles serde_json failures (the block around the second from_value/to_value
usage mentioned in the comment).
- Around line 793-808: The examples use bare names for function IDs; update the
rustdoc snippets that call iii.register_function to use the repository's
::-separated ID convention (e.g., replace "greet" and "echo" with something like
"greet::default" or domain-style IDs such as "orders::validate" /
"reports::daily-summary") so the examples follow the engine-standard function ID
format; locate the calls to iii.register_function in the examples around
RegisterFunction::new_async and change the string IDs accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4f2b220f-83aa-4364-a70d-d4a07da8fcbb
📒 Files selected for processing (3)
docs/changelog/0-11-0/align-rust-register-function-with-signature.mdxsdk/packages/rust/iii/src/iii.rssdk/packages/rust/iii/tests/queue_integration.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/changelog/0-11-0/align-rust-register-function-with-signature.mdx
- sdk/packages/rust/iii/tests/queue_integration.rs
guibeira
changed the title
andersonleal
previously approved these changes
May 1, 2026
Mirror of Node's RegisterFunctionOptions: description, request_format, response_format, metadata. Will back the reshaped register_function_with signature in the next commit.
Reshape register_function_with from (message, handler) to
(id, handler, options) so the cross-SDK call shape matches:
Node: registerFunction(id, handler, options?)
Python: register_function(function_id, handler, *, description, ...)
Rust: register_function_with(id, handler, options)
register_function(id, f) and the tuple form
register_function((message, handler)) are unchanged.
BREAKING CHANGE: register_function_with argument order changed.
See docs/changelog/0-11-0/align-rust-register-function-with-signature.mdx
for migration.
Reflect new (id, handler, options) signature.
The Before cell used Default::default() on RegisterFunctionMessage, which does not derive Default. Replace with the existing .with_id(...).with_description(...) builder.
The reshaped register_function_with(id, handler, options) signature references RegisterFunctionOptions. Add it to EXPORTED_TYPES so its section renders in the regenerated API reference.
Drop reg_fn_msg helper and update all 32 call sites to the new (id, handler, options) shape after iii-sdk's BREAKING change. BREAKING CHANGE: registration call sites changed shape; no behavioral change.
Reflect new (id, handler, options) signature in the SKILL.md table and the http-invoked-functions.rs reference snippet.
The auto-generated SDK reference dropped HttpInvocationConfig, HttpAuthConfig, HttpMethod, FunctionInfo, TriggerInfo, WorkerInfo, and RegisterFunctionMessage sections after register_function_with's signature stopped naming HttpInvocationConfig directly. Those types are still referenced in method/type doc-comments via intra-doc links (e.g., 'Async closure or [HttpInvocationConfig]'). Extend filterReferencedTypes to also scan description fields and examples, so types named in prose stay in the rendered output. Symmetric fix lands sdk-python.mdx changes too: ChannelReader, ChannelWriter, TriggerAction, EnqueueResult, StreamChannelRef, EngineLogExporter, EngineMetricsExporter, Trigger, TriggerTypeRef, and Channel are also referenced only in prose (e.g. ':meth:' / '\`\`trigger()\`\`') and were dropped for the same reason.
Replace the prior register_function / register_function_with /
RegisterFunction tuple split with a single API:
iii.register_function(id, RegisterFunction::new_async(fetch).description(...));
Constructors carry the registration kind:
RegisterFunction::new(f) // sync typed (auto-extracted schemas)
RegisterFunction::new_async(f) // async typed (auto-extracted schemas)
RegisterFunction::raw(f) // async untyped Value handler
RegisterFunction::http(config) // HTTP-invoked
Builder methods (chainable, all consume self):
.description(s) .metadata(v)
.request_format(schema) .response_format(schema)
When request_format / response_format are set on the builder they
override any auto-extracted schemas.
Removed:
* register_function_with
* register_function((RegisterFunctionMessage, handler)) tuple form
* RegisterFunctionOptions
* IntoFunctionRegistration trait
* IntoFunctionHandler trait + impls
* iii_fn / iii_async_fn / IIIFn / IIIAsyncFn
Migrated all in-tree consumers: SDK integration tests, iii-example
crate, console-rust (33 callers), and three engine bridges. Skill
references and the changelog entry rewritten. docs/api-reference/
sdk-rust.mdx regenerated.
BREAKING CHANGE: register_function_with, RegisterFunctionOptions,
the (RegisterFunctionMessage, handler) tuple form, IntoFunctionHandler,
IntoFunctionRegistration, iii_fn/iii_async_fn/IIIFn/IIIAsyncFn are all
removed. See docs/changelog/0-11-0/align-rust-register-function-with-signature.mdx
for the migration table.
CI surfaced two issues with the prior commit:
1. crates/iii-worker/src/sandbox_daemon/{mod,fs/*}.rs (14 sites)
were still using the old register_function_with(message, handler)
signature — never picked up by the previous migration sweep, which
only searched sdk/, console/, engine/, and skills/. Migrated to the
new register_function(id, RegisterFunction::raw(handler).description(...))
shape.
2. The migration script's output had inconsistent spacing that
tripped cargo fmt --check across the workspace. Ran cargo fmt --all
to fix.
Verified locally:
- cargo build --workspace --all-targets — clean
- cargo fmt --all -- --check — clean
- cargo test -p iii-sdk --lib — 64/64 pass
- cargo test -p iii-sdk --doc — 16/16 pass
The 'untyped' name is more self-explanatory: it pairs cleanly with the typed 'new' / 'new_async' constructors and signals 'no schema introspection' rather than implying low-level access. Pure mechanical rename across all consumers, the changelog, the API reference, and skill docs. No behavioral change.
… IIIError
Drop RegisterFunction::untyped — `serde_json::Value: JsonSchema` (via
schemars), so `RegisterFunction::new` and `::new_async` already accept
both typed handlers AND `Fn(Value) -> ...` closures. The `untyped`
constructor was redundant.
To make this work without forcing every caller to write
`Ok::<_, IIIError>(...)` annotations, fix the handler error type to
`IIIError` (was generic `E: Display`):
// Before
F: Fn(T) -> Result<R, E>
E: Display + Send + 'static
// After
F: Fn(T) -> Result<R, IIIError>
Add `From<String>` and `From<&str>` for `IIIError` to smooth the
migration of existing handlers returning `Result<R, String>`.
Constructors are now 3:
- `RegisterFunction::new(f)` — sync, typed or Value
- `RegisterFunction::new_async(f)` — async, typed or Value
- `RegisterFunction::http(config)` — HTTP-invoked
Migration:
- `RegisterFunction::untyped(f)` -> `RegisterFunction::new_async(f)` (~118 sites)
- `Result<R, String>` -> `Result<R, IIIError>` (~40 sites in iii-example, tests)
- Closures with implicit `_input` -> `_input: Value` for inference (~3 sites)
BREAKING CHANGE: handler signatures now require `Result<_, IIIError>`.
Use `IIIError::Handler(s)`, `Err(s.into())`, or `?`-propagation instead
of `Err(string)`.
- queue_integration.rs: switch durable test function IDs from dot- separated (test.queue.foo.rs.X) to ::-separated (test::queue::foo::rs::X) to match the repo standard. - iii.rs: drop the duplicated RegisterFunction::new_async entry from the register_function rustdoc (the API has 3 constructors, not 4). - changelog: distinguish sync vs async replacements in the migration table — register_function_with and the tuple form map to either RegisterFunction::new or RegisterFunction::new_async depending on the handler kind. - changelog: fix Python signature parameter name from `handler` to `handler_or_invocation` (keeping the `*` keyword-only marker since the real Python API does require it).
- IntoSyncHandler / IntoAsyncHandler: map serde_json::from_value and
serde_json::to_value failures to IIIError::Serde instead of
IIIError::Handler. Preserves the distinction between user handler
errors and SDK-level (de)serialization errors.
- iii.rs rustdoc: replace bare ids "greet" / "echo" in the
register_function examples with the repo's ::-separated convention
("greetings::greet" / "debug::echo") to match other examples.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/iii-worker/src/sandbox_daemon/fs/mkdir.rs (1)
82-86:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMap serde_json errors to the correct IIIError variant.
Both deserialization (lines 82-83) and serialization (lines 85-86) errors are currently wrapped in
IIIError::Handler, but according to the PR commit message, "serde_json (de)serialization failures [should map] to IIIError::Serde (preserving distinction between handler and SDK errors)." SinceIIIErrorimplementsFrom<serde_json::Error>, you can use the?operator directly to leverage the automatic conversion.This distinction is important for observability: SDK/serialization errors should be categorized separately from business-logic handler errors.
🔧 Proposed fix
let registry = registry.clone(); let runner = runner.clone(); Box::pin(async move { - let req: MkdirRequest = serde_json::from_value(payload) - .map_err(|e| IIIError::Handler(format!("bad request: {e}")))?; + let req: MkdirRequest = serde_json::from_value(payload)?; match handle_mkdir(req, ®istry, &*runner).await { - Ok(resp) => serde_json::to_value(resp) - .map_err(|e| IIIError::Handler(format!("serialize: {e}"))), + Ok(resp) => Ok(serde_json::to_value(resp)?), Err(e) => Err(IIIError::Handler( serde_json::to_string(&e.to_payload()).unwrap_or_else(|_| e.to_string()), )), }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/iii-worker/src/sandbox_daemon/fs/mkdir.rs` around lines 82 - 86, The serde_json (de)serialization errors in the Mkdir RPC are incorrectly wrapped as IIIError::Handler; change the deserialization and serialization calls so they use the ? operator and let serde_json::Error convert into IIIError via its From impl (i.e., replace the explicit map_err(...) for serde_json::from_value(payload) and serde_json::to_value(resp) with direct ? usage), keeping handler errors from handle_mkdir(...) unchanged; look for MkdirRequest, serde_json::from_value, handle_mkdir, serde_json::to_value and remove the IIIError::Handler mapping so serde errors become IIIError::Serde.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/iii-worker/src/sandbox_daemon/fs/mkdir.rs`:
- Around line 82-86: The serde_json (de)serialization errors in the Mkdir RPC
are incorrectly wrapped as IIIError::Handler; change the deserialization and
serialization calls so they use the ? operator and let serde_json::Error convert
into IIIError via its From impl (i.e., replace the explicit map_err(...) for
serde_json::from_value(payload) and serde_json::to_value(resp) with direct ?
usage), keeping handler errors from handle_mkdir(...) unchanged; look for
MkdirRequest, serde_json::from_value, handle_mkdir, serde_json::to_value and
remove the IIIError::Handler mapping so serde errors become IIIError::Serde.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f06ffdee-2a0a-45bd-8b00-9ebf32bf3300
📒 Files selected for processing (22)
console/packages/console-rust/src/bridge/functions.rsconsole/packages/console-rust/src/main.rscrates/iii-worker/src/cli/worker_manager_daemon.rscrates/iii-worker/src/sandbox_daemon/fs/chmod.rscrates/iii-worker/src/sandbox_daemon/fs/grep.rscrates/iii-worker/src/sandbox_daemon/fs/ls.rscrates/iii-worker/src/sandbox_daemon/fs/mkdir.rscrates/iii-worker/src/sandbox_daemon/fs/mv.rscrates/iii-worker/src/sandbox_daemon/fs/read.rscrates/iii-worker/src/sandbox_daemon/fs/rm.rscrates/iii-worker/src/sandbox_daemon/fs/sed.rscrates/iii-worker/src/sandbox_daemon/fs/stat.rscrates/iii-worker/src/sandbox_daemon/fs/write.rscrates/iii-worker/src/sandbox_daemon/mod.rsdocs/api-reference/sdk-node.mdxdocs/api-reference/sdk-python.mdxdocs/api-reference/sdk-rust.mdxdocs/changelog/0-11-0/align-rust-register-function-with-signature.mdxdocs/scripts/generate-api-docs.mtsdocs/scripts/parsers/parse-rustdoc.mtsengine/src/workers/bridge_client/mod.rsengine/src/workers/queue/adapters/bridge.rs
💤 Files with no reviewable changes (1)
- console/packages/console-rust/src/main.rs
✅ Files skipped from review due to trivial changes (2)
- docs/api-reference/sdk-node.mdx
- docs/changelog/0-11-0/align-rust-register-function-with-signature.mdx
🚧 Files skipped from review as they are similar to previous changes (17)
- crates/iii-worker/src/sandbox_daemon/fs/write.rs
- crates/iii-worker/src/sandbox_daemon/fs/rm.rs
- crates/iii-worker/src/sandbox_daemon/fs/chmod.rs
- crates/iii-worker/src/sandbox_daemon/fs/sed.rs
- crates/iii-worker/src/sandbox_daemon/fs/read.rs
- docs/scripts/parsers/parse-rustdoc.mts
- console/packages/console-rust/src/bridge/functions.rs
- crates/iii-worker/src/sandbox_daemon/fs/stat.rs
- crates/iii-worker/src/sandbox_daemon/fs/ls.rs
- crates/iii-worker/src/sandbox_daemon/fs/grep.rs
- engine/src/workers/bridge_client/mod.rs
- engine/src/workers/queue/adapters/bridge.rs
- crates/iii-worker/src/sandbox_daemon/mod.rs
- docs/scripts/generate-api-docs.mts
- docs/api-reference/sdk-python.mdx
- crates/iii-worker/src/sandbox_daemon/fs/mv.rs
- docs/api-reference/sdk-rust.mdx
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
2 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
Collapses the Rust SDK's function-registration surface to a single entry point that mirrors Node and Python:
registerFunction(functionId, handler, options?)register_function(function_id, handler, *, description, metadata, request_format, response_format)register_function(id, RegisterFunction)RegisterFunctioncarries the handler plus all optional metadata. There are three constructors:RegisterFunction::new(f)Fn(Value) -> Result<Value, IIIError>RegisterFunction::new_async(f)new— same dual supportRegisterFunction::http(config)ValueimplementsJsonSchema(viaschemars), so untyped handlers go throughnew/new_asyncand emit a permissiveAnyValueschema. No separateuntypedconstructor needed.Builder methods (chainable, consume
self):.description(s),.metadata(v),.request_format(schema),.response_format(schema). Setters override any auto-extracted schema.Handler error type fixed to
IIIErrorTo enable clean type inference for
Fn(Value) -> ...closures (whereOk(...)alone left the error type unbound), the handler bound is fixed:To smooth migration,
IIIErrornow implementsFrom<String>andFrom<&str>(alongside the existingFrom<serde_json::Error>andFrom<tungstenite::Error>). Existing handlers can:Result<R, IIIError>and useIIIError::Handler(s)directly, OR?-propagation withErr(\"oops\")?/Err(\"oops\".to_string())?, ORResult<R, String>toResult<R, IIIError>with no body changes (string literals work as errors viaFrom<&str>).What was removed
III::register_function_with(id, handler, options)register_function(id, RegisterFunction::new_async(handler).description(...))register_function((RegisterFunctionMessage, handler))tuple formregister_function(id, RegisterFunction::new_async(handler))RegisterFunction::untyped(f)RegisterFunction::new_async(f)(now acceptsValueclosures directly)RegisterFunctionOptionsRegisterFunctionIntoFunctionRegistrationtraitRegisterFunctionargumentIntoFunctionHandlertrait + implsRegisterFunction::*constructoriii_fn,iii_async_fn,IIIFn,IIIAsyncFnRegisterFunction::new/RegisterFunction::new_asyncE: DisplayResult<_, IIIError>; useFrom<String>/From<&str>to liftIn-tree migration
All call sites migrated:
sdk/packages/rust/iii/tests/*.rs— integration tests (~57 sites).sdk/packages/rust/iii-example/src/*.rs— example crate;Result<R, String>->Result<R, IIIError>.console/packages/console-rust/src/bridge/functions.rs— 33 sites;reg_fn_msghelper retired.engine/src/workers/{bridge_client,queue/adapters/bridge,stream/adapters/bridge}.rs— 3 sites.crates/iii-worker/src/sandbox_daemon/{mod,fs/*}.rs— 14 sites.skills/iii-rust-sdk/SKILL.md,skills/references/http-invoked-functions.rs— teaching references.docs/changelog/0-11-0/align-rust-register-function-with-signature.mdx— rewritten with the final shape.docs/api-reference/sdk-rust.mdx— regenerated.BREAKING CHANGE
register_function_with,RegisterFunctionOptions,IntoFunctionRegistration,IntoFunctionHandler,iii_fn,iii_async_fn,IIIFn,IIIAsyncFn, the(RegisterFunctionMessage, handler)tuple form, and theuntypedconstructor are all removed. Handler error type must beIIIError. See the changelog entry for the migration table.Test plan
Notes
builtins::kv::test_builtin_kv_lock_ttl_expiry,test_file_based_preserves_insertion_order) fail under workspace-wide test load due to tight 10-20 ms timing windows; pass when run in isolation. Unrelated to this branch and pass in CI.crates/iii-supervisor/src/shell_protocol.rs(`set_len` after reserve) and engine code are not enforced by CI for those crates and are unrelated to this branch.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores