feat: consolidate registerFunction to accept handler or HttpInvocationConfig#38
Closed
feat: consolidate registerFunction to accept handler or HttpInvocationConfig#38
Conversation
…nConfig Removes registerHttpFunction/register_http_function from all three SDKs (Node, Python, Rust) and unifies the API so registerFunction accepts either a local handler or an HttpInvocationConfig for HTTP-invoked functions. Also adds Rust e2e integration tests for HTTP external functions and a developer Makefile.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughConsolidates HTTP and local function registration across Node, Python, and Rust SDKs into a single registerFunction/register_function API that accepts either a handler or an HTTP invocation config; removes separate HTTP-function storage. Adds a top-level Makefile with engine lifecycle, tooling, tests, and CI workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant SDK as SDK (Node / Python / Rust)
participant Engine as III Engine (WebSocket)
participant HTTP as External HTTP Endpoint
SDK->>Engine: registerFunction(RegisterFunctionMessage with optional invocation)
Engine-->>SDK: ack / Registered
SDK->>Engine: enqueue event / trigger
Engine->>HTTP: HTTP request to invocation.url (method, headers, body)
HTTP-->>Engine: HTTP response (status, body)
Engine-->>SDK: InvocationResult (success/failure)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/node/iii/tests/http-external-functions.test.ts (1)
145-149:⚠️ Potential issue | 🟡 MinorFix Biome formatting to resolve CI failure.
The pipeline indicates Biome would reformat this code block. Run the formatter to fix.
🔧 Fix by running formatter
npx `@biomejs/biome` check --fix packages/node/iii/tests/http-external-functions.test.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/node/iii/tests/http-external-functions.test.ts` around lines 145 - 149, The test file is failing CI because Biome reformats the http function registration block; run the Biome formatter (npx `@biomejs/biome` check --fix packages/node/iii/tests/http-external-functions.test.ts) or apply the formatter changes so the iii.registerFunction call (variable httpFn, method iii.registerFunction, and webhookProbe.url()) is properly formatted to satisfy Biome and eliminate the CI formatting error.
🧹 Nitpick comments (2)
Makefile (1)
10-15: Consider adding conventionalallandcleantargets.The static analysis tool flags missing
allandcleanphony targets. While not strictly required, these are conventional Makefile targets that improve discoverability.cleancould alias toengine-clean, andallcould default tohelporsetup.🔧 Proposed addition
.PHONY: help engine-build engine-start engine-stop engine-status engine-logs engine-clean .PHONY: setup setup-node setup-python .PHONY: lint lint-node lint-python lint-rust .PHONY: test test-node test-python test-rust .PHONY: integration integration-node integration-python integration-rust .PHONY: ci ci-node ci-python ci-rust +.PHONY: all clean + +all: help + +clean: engine-clean🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 10 - 15, Add conventional phony targets "all" and "clean" to the Makefile by declaring them in the .PHONY list and implementing them: define "all" as the default target that depends on "help" (or "setup" if you prefer) so running `make` shows the help/setup behaviour, and implement "clean" as a simple alias that invokes the existing "engine-clean" target (e.g., "clean: engine-clean"); ensure both "all" and "clean" are included in the .PHONY declaration alongside the existing targets like engine-clean, help, and setup.packages/rust/iii/tests/http_external_functions.rs (1)
63-68: Singleread()may not receive complete HTTP request.TCP can fragment data across multiple packets. A single
read()call may return partial data, causing the webhook to capture an incomplete request. For test reliability, consider reading in a loop until the header terminator (\r\n\r\n) is found or the connection closes.🔧 Proposed fix for more reliable reading
async fn accept_one(&self) -> CapturedWebhook { let (mut stream, _) = self.listener.accept().await.expect("accept failed"); - let mut buf = vec![0u8; 8192]; - let n = stream.read(&mut buf).await.expect("read failed"); - let raw = String::from_utf8_lossy(&buf[..n]).to_string(); + let mut buf = Vec::with_capacity(8192); + let mut temp = [0u8; 4096]; + loop { + let n = stream.read(&mut temp).await.expect("read failed"); + if n == 0 { + break; + } + buf.extend_from_slice(&temp[..n]); + // Check if we've received the end of headers + if buf.windows(4).any(|w| w == b"\r\n\r\n") { + break; + } + } + let raw = String::from_utf8_lossy(&buf).to_string();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rust/iii/tests/http_external_functions.rs` around lines 63 - 68, The single read in async fn accept_one currently may return a partial HTTP request; update accept_one (and related CapturedWebhook handling) to read from the accepted stream in a loop (using stream.read until it returns 0) appending into buf and stop once the HTTP header terminator "\r\n\r\n" is found or the socket closes, then convert the accumulated bytes to a String (raw) for parsing; ensure you still respect the existing 8192 initial buffer sizing logic but grow/append as needed to avoid truncation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Line 71: The Makefile line that uses lsof to free III_HEALTH_PORT can fail on
minimal images; modify the health-port cleanup in the Makefile (the rule
invoking lsof -ti :$(III_HEALTH_PORT)) to either implement a fallback path that
checks for and uses ss, fuser, or /proc/net/tcp if lsof is absent, or add a
clear prerequisite note in the README/Makefile header that lsof is a required
dependency; ensure the change gracefully handles missing tools (try lsof, then
ss, then fuser, then /proc) and still kills the PID if found, or document the
requirement and exit with a descriptive error if none are available.
In `@packages/node/iii/src/iii.ts`:
- Around line 813-823: The code currently calls
this.sendMessage(MessageType.InvocationResult, ...) even when invocation_id is
undefined; update the logic in the branch that computes errorCode/errorMessage
(around invocation_id, function_id, MessageType.InvocationResult) to first check
that invocation_id is present and only call this.sendMessage when it is
truthy—if invocation_id is absent simply skip sending the InvocationResult
(return or end the function); keep the existing error payload construction
(errorCode/errorMessage) but guard the send to avoid emitting an invalid
response for fire-and-forget invocations.
In `@packages/python/iii/src/iii/iii.py`:
- Around line 409-412: The long inline assignment to error_msg in the if-block
(when checking func and func.handler in function that defines
error_code/error_msg and calls log.warning) exceeds line length; refactor by
splitting the ternary-like construction into a multi-line conditional or assign
the message to error_msg using an if/else block (referencing variables func,
path, error_code, error_msg and the log.warning call) so each line stays under
the 120-character limit while preserving the exact messages: "Function is
HTTP-invoked and cannot be invoked locally" when func exists but not invokable,
otherwise f"Function '{path}' not found".
- Around line 572-589: The code treats any non-HttpInvocationConfig as a handler
without validating it; add a type check to ensure handler_or_invocation is
callable before registering it as a handler. In the branch where is_handler is
True (variables: handler_or_invocation, is_handler), verify
callable(handler_or_invocation) and raise a clear TypeError (or ValueError) if
not callable, then proceed to create msg, wrap the handler (wrapped) and store
RemoteFunctionData(message=msg, handler=wrapped); keep the existing behavior for
the else branch that handles HttpInvocationConfig (msg =
RegisterFunctionMessage(..., invocation=config) and self._functions[path] =
RemoteFunctionData(message=msg). Ensure the validation happens before
creating/storing the RemoteFunctionData so invalid values never get registered.
In `@packages/rust/iii/src/iii.rs`:
- Around line 392-397: register_function_with_description and
register_function_with bypass the empty-id validation that register_function
enforces; centralize the non-empty id check inside register_function_inner so
all callers are validated. Modify register_function_inner to validate message.id
(or the cloned id) is non-empty and return/handle the error consistently, and
remove any duplicate checks from register_function_with_description and
register_function_with so they delegate to register_function_inner for id
validation and registration logic.
- Around line 413-418: Run rustfmt to fix CI style failures by reformatting the
affected block around the closure creating unregister_fn and the nearby code at
lines ~1063 and ~1066-1071; ensure the closure that references unregister_fn,
iii.inner.functions.lock_or_recover(), iii.send_message,
Message::UnregisterFunction, and unregister_id is formatted according to rustfmt
(correct indentation, spacing, and trailing commas), then re-run CI to confirm
formatting errors are resolved.
- Around line 398-408: The duplicate check and insert on self.inner.functions
currently lock twice (contains_key then insert) allowing a race; acquire the
mutex once by calling let mut funcs = self.inner.functions.lock_or_recover() and
perform the check+insert while holding that guard (use the HashMap entry API on
funcs: match funcs.entry(id.clone()) { Occupied => panic!("function id '{}'
already registered", id), Vacant => insert RemoteFunctionData { message:
message.clone(), handler } } ) so the contains/insert are atomic under a single
lock.
In `@packages/rust/iii/src/lib.rs`:
- Around line 19-22: The pub use import block (exporting FunctionInfo,
FunctionRef, FunctionsAvailableGuard, IntoFunctionHandler, III, TriggerInfo,
WorkerInfo, WorkerMetadata) is not in rustfmt's canonical order and CI fails;
run the Rust formatter (cd into packages/rust/iii and run cargo fmt --all) or
reorder the items to match rustfmt's ordering so the re-export list is formatted
as cargo fmt would produce.
In `@packages/rust/iii/tests/http_external_functions.rs`:
- Around line 117-121: The CI is failing due to rustfmt style issues; run the
Rust formatter in the iii crate and commit the changes: from the repository root
run the formatter for the package (e.g., cd packages/rust/iii && cargo fmt
--all) to reformat files including the test file where CapturedWebhook, the
response assignment, and stream.write_all appear; ensure the resulting changes
are staged and committed so rustfmt fixes on lines around those symbols are
included in the PR.
---
Outside diff comments:
In `@packages/node/iii/tests/http-external-functions.test.ts`:
- Around line 145-149: The test file is failing CI because Biome reformats the
http function registration block; run the Biome formatter (npx `@biomejs/biome`
check --fix packages/node/iii/tests/http-external-functions.test.ts) or apply
the formatter changes so the iii.registerFunction call (variable httpFn, method
iii.registerFunction, and webhookProbe.url()) is properly formatted to satisfy
Biome and eliminate the CI formatting error.
---
Nitpick comments:
In `@Makefile`:
- Around line 10-15: Add conventional phony targets "all" and "clean" to the
Makefile by declaring them in the .PHONY list and implementing them: define
"all" as the default target that depends on "help" (or "setup" if you prefer) so
running `make` shows the help/setup behaviour, and implement "clean" as a simple
alias that invokes the existing "engine-clean" target (e.g., "clean:
engine-clean"); ensure both "all" and "clean" are included in the .PHONY
declaration alongside the existing targets like engine-clean, help, and setup.
In `@packages/rust/iii/tests/http_external_functions.rs`:
- Around line 63-68: The single read in async fn accept_one currently may return
a partial HTTP request; update accept_one (and related CapturedWebhook handling)
to read from the accepted stream in a loop (using stream.read until it returns
0) appending into buf and stop once the HTTP header terminator "\r\n\r\n" is
found or the socket closes, then convert the accumulated bytes to a String (raw)
for parsing; ensure you still respect the existing 8192 initial buffer sizing
logic but grow/append as needed to avoid truncation.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
Makefilepackages/node/iii/src/iii.tspackages/node/iii/src/types.tspackages/node/iii/tests/http-external-functions.test.tspackages/python/iii/src/iii/iii.pypackages/python/iii/src/iii/types.pypackages/python/iii/tests/test_http_external_functions_integration.pypackages/rust/iii/src/iii.rspackages/rust/iii/src/lib.rspackages/rust/iii/src/types.rspackages/rust/iii/tests/http_external_functions.rs
Comment on lines
+392
to
+397
| fn register_function_inner( | ||
| &self, | ||
| id: impl Into<String>, | ||
| description: impl Into<String>, | ||
| handler: F, | ||
| ) where | ||
| F: Fn(Value) -> Fut + Send + Sync + 'static, | ||
| Fut: std::future::Future<Output = Result<Value, IIIError>> + Send + 'static, | ||
| { | ||
| let message = RegisterFunctionMessage { | ||
| id: id.into(), | ||
| description: Some(description.into()), | ||
| request_format: None, | ||
| response_format: None, | ||
| metadata: None, | ||
| invocation: None, | ||
| }; | ||
|
|
||
| self.register_function_with(message, handler); | ||
| } | ||
|
|
||
| pub fn register_function_with<F, Fut>(&self, message: RegisterFunctionMessage, handler: F) | ||
| where | ||
| F: Fn(Value) -> Fut + Send + Sync + 'static, | ||
| Fut: std::future::Future<Output = Result<Value, IIIError>> + Send + 'static, | ||
| { | ||
| if self | ||
| .inner | ||
| .http_functions | ||
| .lock_or_recover() | ||
| .contains_key(&message.id) | ||
| { | ||
| panic!( | ||
| "function id '{}' already registered as HTTP function", | ||
| message.id | ||
| ); | ||
| message: RegisterFunctionMessage, | ||
| handler: Option<RemoteFunctionHandler>, | ||
| ) -> FunctionRef { | ||
| let id = message.id.clone(); |
There was a problem hiding this comment.
Centralize non-empty id validation in register_function_inner.
register_function_with_description and register_function_with currently bypass the empty-id check. This allows invalid registrations that register_function rejects.
💡 Suggested fix
fn register_function_inner(
&self,
message: RegisterFunctionMessage,
handler: Option<RemoteFunctionHandler>,
) -> FunctionRef {
let id = message.id.clone();
+ if id.trim().is_empty() {
+ panic!("id is required");
+ }
if self.inner.functions.lock_or_recover().contains_key(&id) {
panic!("function id '{}' already registered", id);
}Also applies to: 447-472
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rust/iii/src/iii.rs` around lines 392 - 397,
register_function_with_description and register_function_with bypass the
empty-id validation that register_function enforces; centralize the non-empty id
check inside register_function_inner so all callers are validated. Modify
register_function_inner to validate message.id (or the cloned id) is non-empty
and return/handle the error consistently, and remove any duplicate checks from
register_function_with_description and register_function_with so they delegate
to register_function_inner for id validation and registration logic.
Comment on lines
+413
to
+418
| let unregister_fn = Arc::new(move || { | ||
| let _ = iii.inner.functions.lock_or_recover().remove(&unregister_id); | ||
| let _ = iii.send_message(Message::UnregisterFunction { | ||
| id: unregister_id.clone(), | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Run rustfmt to resolve CI style failures.
CI already flags formatting drift around Line 417 and Line 1063.
Also applies to: 1066-1071
🧰 Tools
🪛 GitHub Actions: CI - Rust
[error] 417-417: rustfmt style changes detected in FunctionRef block. Run 'cargo fmt' to apply formatting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rust/iii/src/iii.rs` around lines 413 - 418, Run rustfmt to fix CI
style failures by reformatting the affected block around the closure creating
unregister_fn and the nearby code at lines ~1063 and ~1066-1071; ensure the
closure that references unregister_fn, iii.inner.functions.lock_or_recover(),
iii.send_message, Message::UnregisterFunction, and unregister_id is formatted
according to rustfmt (correct indentation, spacing, and trailing commas), then
re-run CI to confirm formatting errors are resolved.
- Enhanced the Makefile to support additional methods for checking running processes on the specified health port. - Updated the Sdk class in Node.js to ensure error messages are only sent if an invocation ID is present. - Refactored function registration in Rust to include validation for empty IDs and improved error handling for duplicate registrations. - Cleaned up formatting in various test files for better readability and consistency.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
Makefile (2)
10-15: Consider adding conventionalallandcleantargets.Static analysis flags missing
allandcleanphony targets. Whileengine-cleanexists, addingall(defaulting tohelpor a common workflow) and aliasingcleantoengine-cleanwould align with Makefile conventions and improve discoverability..PHONY: help engine-build engine-start engine-stop engine-status engine-logs engine-clean .PHONY: setup setup-node setup-python .PHONY: lint lint-node lint-python lint-rust .PHONY: test test-node test-python test-rust .PHONY: integration integration-node integration-python integration-rust .PHONY: ci ci-node ci-python ci-rust +.PHONY: all clean + +all: help + +clean: engine-clean🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 10 - 15, Add conventional Makefile targets by introducing an all target (default) that depends on help or the main workflow and a clean alias that invokes the existing engine-clean; update .PHONY to include all and clean so they are treated as phony targets, ensure the new all target is placed before other rules to become the default, and implement clean as a simple dependency or recipe that delegates to engine-clean (referencing the existing engine-clean and help targets).
79-86: Add fallback forncin health check or document as dependency.Similar to the lsof fallback at line 71,
nc -z(netcat) may not be available in minimal container images. Consider adding a fallback using bash's built-in/dev/tcppseudo-device orcurl/wgetif the engine exposes an HTTP health endpoint.♻️ Suggested fallback using /dev/tcp
`@for` i in $$(seq 1 30); do \ - if nc -z 127.0.0.1 $(III_HEALTH_PORT) 2>/dev/null; then \ + if nc -z 127.0.0.1 $(III_HEALTH_PORT) 2>/dev/null || \ + (exec 3<>/dev/tcp/127.0.0.1/$(III_HEALTH_PORT)) 2>/dev/null; then \ echo "III Engine is ready!"; exit 0; \ fi; \ echo " attempt $$i/30..."; sleep 2; \ done; \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 79 - 86, The health-check loop in the Makefile relies solely on `nc -z`, which may be missing in minimal containers; update the loop that references III_HEALTH_PORT and III_LOG_FILE to try fallbacks: first check for `nc` (command -v nc), then try bash `/dev/tcp/127.0.0.1/$(III_HEALTH_PORT)` as a TCP probe, then try `curl --fail` or `wget --spider` if an HTTP endpoint is expected, and if none are available emit a clear error and exit non-zero; make the check sequence part of the same retry loop so the existing messages ("attempt $$i/30...", "Engine is ready!", and the tail of $(III_LOG_FILE)") remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/python/iii/src/iii/iii.py`:
- Around line 575-597: The union narrowing fails because register_function uses
"isinstance(..., HttpInvocationConfig) is False"; change the condition to
"isinstance(handler_or_invocation, HttpInvocationConfig)" and swap the then/else
branches so the branch where handler_or_invocation is an HttpInvocationConfig is
the positive branch (set config = cast(HttpInvocationConfig,
handler_or_invocation), construct msg with invocation=config, and set
_functions[path] = RemoteFunctionData(message=msg)), while the other branch
handles the callable case (validate callable, wrap it with wrapped async
function, create RegisterFunctionMessage without invocation, and set
_functions[path] = RemoteFunctionData(message=msg, handler=wrapped)); also add
"cast" to the imports from typing and use cast when assigning config to satisfy
mypy strict mode.
In `@packages/rust/iii/tests/http_external_functions.rs`:
- Around line 66-115: The current single-read logic in WebhookProbe::accept_one
(using a one-shot stream.read into buf and then parsing headers/body) can miss
data split across TCP packets; change it to repeatedly read and append into a
growing buffer until the end-of-headers marker is found, then parse headers to
get Content-Length (or handle chunked if needed for tests) and continue reading
more from stream until the full body length is received before deserializing
into body; update uses of
buf/raw/raw_bytes/header_end/body_start/body_bytes/body to operate on the
accumulated buffer and only call serde_json::from_slice once the complete body
is present.
- Around line 117-118: The test builds a fake HTTP response in the variable
response and writes it with stream.write_all, but the Content-Length header is
wrong (declares 10 while body "{\"ok\":true}" is 11 bytes); update the response
string used in tests (the response variable in tests/http_external_functions.rs)
to use the correct Content-Length (11) or construct the response by computing
the body byte length so the Content-Length header matches the actual body before
calling stream.write_all.
---
Nitpick comments:
In `@Makefile`:
- Around line 10-15: Add conventional Makefile targets by introducing an all
target (default) that depends on help or the main workflow and a clean alias
that invokes the existing engine-clean; update .PHONY to include all and clean
so they are treated as phony targets, ensure the new all target is placed before
other rules to become the default, and implement clean as a simple dependency or
recipe that delegates to engine-clean (referencing the existing engine-clean and
help targets).
- Around line 79-86: The health-check loop in the Makefile relies solely on `nc
-z`, which may be missing in minimal containers; update the loop that references
III_HEALTH_PORT and III_LOG_FILE to try fallbacks: first check for `nc` (command
-v nc), then try bash `/dev/tcp/127.0.0.1/$(III_HEALTH_PORT)` as a TCP probe,
then try `curl --fail` or `wget --spider` if an HTTP endpoint is expected, and
if none are available emit a clear error and exit non-zero; make the check
sequence part of the same retry loop so the existing messages ("attempt
$$i/30...", "Engine is ready!", and the tail of $(III_LOG_FILE)") remain
unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Makefilepackages/node/iii/src/iii.tspackages/node/iii/tests/http-external-functions.test.tspackages/python/iii/src/iii/iii.pypackages/rust/iii/src/iii.rspackages/rust/iii/src/lib.rspackages/rust/iii/tests/http_external_functions.rs
packages/python/iii/src/iii/iii.py
Outdated
Comment on lines
+575
to
+597
| is_handler = isinstance(handler_or_invocation, HttpInvocationConfig) is False | ||
|
|
||
| if is_handler: | ||
| if not callable(handler_or_invocation): | ||
| actual_type = type(handler_or_invocation).__name__ | ||
| raise TypeError( | ||
| f"handler_or_invocation must be callable or HttpInvocationConfig, got {actual_type}" | ||
| ) | ||
| handler = handler_or_invocation | ||
| msg = RegisterFunctionMessage(id=path, description=description, metadata=metadata) | ||
| self._send_if_connected(msg) | ||
|
|
||
| async def wrapped(input_data: Any) -> Any: | ||
| logger = Logger(function_name=path) | ||
| ctx = Context(logger=logger) | ||
| return await with_context(lambda _: handler(input_data), ctx) | ||
| async def wrapped(input_data: Any) -> Any: | ||
| logger = Logger(function_name=path) | ||
| ctx = Context(logger=logger) | ||
| return await with_context(lambda _: handler(input_data), ctx) | ||
|
|
||
| self._functions[path] = RemoteFunctionData(message=msg, handler=wrapped) | ||
| self._functions[path] = RemoteFunctionData(message=msg, handler=wrapped) | ||
| else: | ||
| config = handler_or_invocation | ||
| msg = RegisterFunctionMessage(id=path, invocation=config, description=description, metadata=metadata) | ||
| self._send_if_connected(msg) | ||
| self._functions[path] = RemoteFunctionData(message=msg) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "iii.py" | head -20Repository: iii-hq/sdk
Length of output: 131
🏁 Script executed:
git ls-files | grep -E "(iii\.py|__init__\.py)" | head -20Repository: iii-hq/sdk
Length of output: 211
🏁 Script executed:
ls -la packages/python/iii/src/iii/ 2>/dev/null || echo "Path may not exist"Repository: iii-hq/sdk
Length of output: 1059
🏁 Script executed:
wc -l packages/python/iii/src/iii/iii.pyRepository: iii-hq/sdk
Length of output: 92
🏁 Script executed:
sed -n '550,620p' packages/python/iii/src/iii/iii.py | cat -nRepository: iii-hq/sdk
Length of output: 3480
🏁 Script executed:
sed -n '1,50p' packages/python/iii/src/iii/iii.py | cat -nRepository: iii-hq/sdk
Length of output: 1764
🏁 Script executed:
rg -A 5 "class RegisterFunctionMessage|RemoteFunctionHandler|class HttpInvocationConfig" packages/python/iii/src/iii/Repository: iii-hq/sdk
Length of output: 4176
🏁 Script executed:
sed -n '1,50p' packages/python/iii/src/iii/iii_types.py | cat -nRepository: iii-hq/sdk
Length of output: 1733
🏁 Script executed:
rg -A 15 "class RegisterFunctionMessage" packages/python/iii/src/iii/iii_types.pyRepository: iii-hq/sdk
Length of output: 663
🏁 Script executed:
rg "from typing import" packages/python/iii/src/iii/iii.pyRepository: iii-hq/sdk
Length of output: 106
🏁 Script executed:
rg "invocation=" packages/python/iii/src/iii/iii.pyRepository: iii-hq/sdk
Length of output: 167
🏁 Script executed:
find . -type f -name "pyproject.toml" -o -name "mypy.ini" | head -10Repository: iii-hq/sdk
Length of output: 135
🏁 Script executed:
rg -B 3 -A 3 "invocation: HttpInvocationConfig" packages/python/iii/src/iii/iii_types.pyRepository: iii-hq/sdk
Length of output: 385
🏁 Script executed:
cat packages/python/iii/pyproject.tomlRepository: iii-hq/sdk
Length of output: 1321
🏁 Script executed:
find . -type f \( -name "*.yml" -o -name "*.yaml" \) | xargs grep -l "mypy" 2>/dev/null | head -5Repository: iii-hq/sdk
Length of output: 145
🏁 Script executed:
cat .github/workflows/*.yml 2>/dev/null | grep -A 10 "mypy" | head -30Repository: iii-hq/sdk
Length of output: 800
🏁 Script executed:
python3 << 'EOF'
from typing import Callable, Any, Awaitable
# Simulate the type situation
RemoteFunctionHandler = Callable[[Any], Awaitable[Any]]
# Current logic - double negative
def current_logic(handler_or_invocation: RemoteFunctionHandler | str):
is_handler = isinstance(handler_or_invocation, str) is False
if is_handler:
# At this point, mypy can't narrow because of the double negative
print(f"Type of handler_or_invocation in if block: would still be union")
else:
# Same issue here
print(f"Type of handler_or_invocation in else block: would still be union")
# Proposed logic - direct isinstance
def proposed_logic(handler_or_invocation: RemoteFunctionHandler | str):
if isinstance(handler_or_invocation, str):
# mypy knows this is str
config = handler_or_invocation
print(f"Type of config in if block: str (narrowed correctly)")
else:
# mypy knows this is RemoteFunctionHandler
print(f"Type of handler_or_invocation in else block: RemoteFunctionHandler (narrowed correctly)")
print("Current logic analysis:")
current_logic(lambda x: None) # type: ignore
print("\nProposed logic analysis:")
proposed_logic(lambda x: None) # type: ignore
EOFRepository: iii-hq/sdk
Length of output: 255
🏁 Script executed:
sed -n '564,604p' packages/python/iii/src/iii/iii.py | cat -nRepository: iii-hq/sdk
Length of output: 2132
Fix union type narrowing in register_function to resolve mypy strict mode errors.
The current code uses a double negative pattern (isinstance(handler_or_invocation, HttpInvocationConfig) is False) that prevents mypy from properly narrowing the union type. At line 32, passing the still-unnarrowed union type to invocation=config violates the field's type constraint HttpInvocationConfig | None.
Restructure the condition to check isinstance(handler_or_invocation, HttpInvocationConfig) directly and swap the branches so type narrowing works correctly. Also add cast to the imports.
🛠️ Proposed fix
-from typing import Any, Awaitable, Callable, Literal
+from typing import Any, Awaitable, Callable, Literal, cast- is_handler = isinstance(handler_or_invocation, HttpInvocationConfig) is False
-
- if is_handler:
+ if isinstance(handler_or_invocation, HttpInvocationConfig):
+ config = handler_or_invocation
+ msg = RegisterFunctionMessage(
+ id=path,
+ invocation=config,
+ description=description,
+ metadata=metadata,
+ )
+ self._send_if_connected(msg)
+ self._functions[path] = RemoteFunctionData(message=msg)
+ else:
if not callable(handler_or_invocation):
actual_type = type(handler_or_invocation).__name__
raise TypeError(
f"handler_or_invocation must be callable or HttpInvocationConfig, got {actual_type}"
)
- handler = handler_or_invocation
+ handler = cast(RemoteFunctionHandler, handler_or_invocation)
msg = RegisterFunctionMessage(id=path, description=description, metadata=metadata)
self._send_if_connected(msg)
async def wrapped(input_data: Any) -> Any:
logger = Logger(function_name=path)
ctx = Context(logger=logger)
return await with_context(lambda _: handler(input_data), ctx)
self._functions[path] = RemoteFunctionData(message=msg, handler=wrapped)
- else:
- config = handler_or_invocation
- msg = RegisterFunctionMessage(id=path, invocation=config, description=description, metadata=metadata)
- self._send_if_connected(msg)
- self._functions[path] = RemoteFunctionData(message=msg)Please re-run uv run mypy src after this change.
🧰 Tools
🪛 GitHub Actions: CI - Python
[error] 595-595: Step 'uv run mypy src' failed. mypy error in src/iii/iii.py:595. Argument 'invocation' to 'RegisterFunctionMessage' has incompatible type 'Callable[[Any], Awaitable[Any]] | HttpInvocationConfig' expected 'HttpInvocationConfig | None' [arg-type].
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/python/iii/src/iii/iii.py` around lines 575 - 597, The union
narrowing fails because register_function uses "isinstance(...,
HttpInvocationConfig) is False"; change the condition to
"isinstance(handler_or_invocation, HttpInvocationConfig)" and swap the then/else
branches so the branch where handler_or_invocation is an HttpInvocationConfig is
the positive branch (set config = cast(HttpInvocationConfig,
handler_or_invocation), construct msg with invocation=config, and set
_functions[path] = RemoteFunctionData(message=msg)), while the other branch
handles the callable case (validate callable, wrap it with wrapped async
function, create RegisterFunctionMessage without invocation, and set
_functions[path] = RemoteFunctionData(message=msg, handler=wrapped)); also add
"cast" to the imports from typing and use cast when assigning config to satisfy
mypy strict mode.
Comment on lines
+66
to
+115
| let mut buf = vec![0u8; 8192]; | ||
| let n = stream.read(&mut buf).await.expect("read failed"); | ||
| let raw = String::from_utf8_lossy(&buf[..n]).to_string(); | ||
|
|
||
| let mut lines = raw.lines(); | ||
| let request_line = lines.next().unwrap_or(""); | ||
| let parts: Vec<&str> = request_line.splitn(3, ' ').collect(); | ||
| let method = parts.first().copied().unwrap_or("POST").to_string(); | ||
| let url = parts.get(1).copied().unwrap_or("/").to_string(); | ||
| let url = url.split('?').next().unwrap_or("/").to_string(); | ||
|
|
||
| let mut headers = HashMap::new(); | ||
| let raw_bytes = &buf[..n]; | ||
|
|
||
| let mut header_end = 0; | ||
| for i in 0..n.saturating_sub(3) { | ||
| if raw_bytes[i] == b'\r' | ||
| && raw_bytes[i + 1] == b'\n' | ||
| && raw_bytes[i + 2] == b'\r' | ||
| && raw_bytes[i + 3] == b'\n' | ||
| { | ||
| header_end = i + 4; | ||
| break; | ||
| } | ||
| } | ||
| if header_end == 0 { | ||
| for i in 0..n.saturating_sub(1) { | ||
| if raw_bytes[i] == b'\n' && raw_bytes[i + 1] == b'\n' { | ||
| header_end = i + 2; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| let body_start = header_end; | ||
|
|
||
| for line in raw.lines().skip(1) { | ||
| if line.is_empty() { | ||
| break; | ||
| } | ||
| if let Some((k, v)) = line.split_once(':') { | ||
| headers.insert(k.trim().to_lowercase(), v.trim().to_string()); | ||
| } | ||
| } | ||
|
|
||
| let body_bytes = &raw_bytes[body_start..]; | ||
| let body = if body_bytes.is_empty() { | ||
| None | ||
| } else { | ||
| serde_json::from_slice(body_bytes).ok() | ||
| }; |
There was a problem hiding this comment.
Avoid single-read request parsing in WebhookProbe::accept_one.
A single read is not sufficient for TCP-framed HTTP requests. Headers/body can arrive in multiple packets, which can make these tests flaky under CI load.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rust/iii/tests/http_external_functions.rs` around lines 66 - 115,
The current single-read logic in WebhookProbe::accept_one (using a one-shot
stream.read into buf and then parsing headers/body) can miss data split across
TCP packets; change it to repeatedly read and append into a growing buffer until
the end-of-headers marker is found, then parse headers to get Content-Length (or
handle chunked if needed for tests) and continue reading more from stream until
the full body length is received before deserializing into body; update uses of
buf/raw/raw_bytes/header_end/body_start/body_bytes/body to operate on the
accumulated buffer and only call serde_json::from_slice once the complete body
is present.
Comment on lines
+117
to
+118
| let response = b"HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: 10\r\n\r\n{\"ok\":true}"; | ||
| let _ = stream.write_all(response).await; |
There was a problem hiding this comment.
Fix malformed HTTP response Content-Length.
{"ok":true} is 11 bytes, but the response declares Content-Length: 10. This can break strict HTTP clients.
🛠️ Proposed fix
- let response = b"HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: 10\r\n\r\n{\"ok\":true}";
- let _ = stream.write_all(response).await;
+ let body = b"{\"ok\":true}";
+ let headers = format!(
+ "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\n\r\n",
+ body.len()
+ );
+ let _ = stream.write_all(headers.as_bytes()).await;
+ let _ = stream.write_all(body).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
| let response = b"HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: 10\r\n\r\n{\"ok\":true}"; | |
| let _ = stream.write_all(response).await; | |
| let body = b"{\"ok\":true}"; | |
| let headers = format!( | |
| "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\n\r\n", | |
| body.len() | |
| ); | |
| let _ = stream.write_all(headers.as_bytes()).await; | |
| let _ = stream.write_all(body).await; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rust/iii/tests/http_external_functions.rs` around lines 117 - 118,
The test builds a fake HTTP response in the variable response and writes it with
stream.write_all, but the Content-Length header is wrong (declares 10 while body
"{\"ok\":true}" is 11 bytes); update the response string used in tests (the
response variable in tests/http_external_functions.rs) to use the correct
Content-Length (11) or construct the response by computing the body byte length
so the Content-Length header matches the actual body before calling
stream.write_all.
- Simplified the registration process for functions by consolidating the handling of HttpInvocationConfig and callable handlers. - Improved error handling for function registration, ensuring clearer distinction between handler types and reducing redundancy in code. - Enhanced the messaging system for function registration to maintain consistency across the SDK.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/python/iii/src/iii/iii.py (1)
576-576:⚠️ Potential issue | 🟡 MinorSplit the long line to fix CI.
Line 576 exceeds the Ruff max length (128 > 120) and is currently failing CI.
💡 Suggested fix
- msg = RegisterFunctionMessage(id=path, invocation=handler_or_invocation, description=description, metadata=metadata) + msg = RegisterFunctionMessage( + id=path, + invocation=handler_or_invocation, + description=description, + metadata=metadata, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/python/iii/src/iii/iii.py` at line 576, The line creating RegisterFunctionMessage assigned to msg is too long; break the argument list across multiple lines to satisfy the Ruff max length—e.g., call RegisterFunctionMessage(id=path, invocation=handler_or_invocation, description=description, metadata=metadata) with each keyword argument on its own line (or split into two shorter lines) so the statement for msg stays under 120 chars while preserving the same identifiers (msg and RegisterFunctionMessage).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/python/iii/src/iii/iii.py`:
- Around line 594-596: Remove the duplicate registration and overwrite that
discards the handler: after setting self._functions[path] =
RemoteFunctionData(message=msg, handler=wrapped) and calling
self._send_if_connected(msg), do not assign self._functions[path] =
RemoteFunctionData(message=msg) again; delete that second assignment so the
handler (wrapped) remains stored and the registration message is not sent twice
(references: _functions, RemoteFunctionData, _send_if_connected, wrapped, msg,
path).
---
Duplicate comments:
In `@packages/python/iii/src/iii/iii.py`:
- Line 576: The line creating RegisterFunctionMessage assigned to msg is too
long; break the argument list across multiple lines to satisfy the Ruff max
length—e.g., call RegisterFunctionMessage(id=path,
invocation=handler_or_invocation, description=description, metadata=metadata)
with each keyword argument on its own line (or split into two shorter lines) so
the statement for msg stays under 120 chars while preserving the same
identifiers (msg and RegisterFunctionMessage).
packages/python/iii/src/iii/iii.py
Outdated
Comment on lines
+594
to
+596
| self._functions[path] = RemoteFunctionData(message=msg, handler=wrapped) | ||
| self._send_if_connected(msg) | ||
| self._functions[path] = RemoteFunctionData(message=msg) |
There was a problem hiding this comment.
Critical bug: Handler is discarded and message sent twice.
Lines 595-596 appear to be leftover code that:
- Sends the registration message a second time (already sent at line 587).
- Overwrites
_functions[path]without thehandler, causing all locally registered functions to fail invocation withfunction_not_invokable.
🐛 Proposed fix: Remove duplicate lines
async def wrapped(input_data: Any) -> Any:
logger = Logger(function_name=path)
ctx = Context(logger=logger)
return await with_context(lambda _: handler(input_data), ctx)
self._functions[path] = RemoteFunctionData(message=msg, handler=wrapped)
- self._send_if_connected(msg)
- self._functions[path] = RemoteFunctionData(message=msg)📝 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
| self._functions[path] = RemoteFunctionData(message=msg, handler=wrapped) | |
| self._send_if_connected(msg) | |
| self._functions[path] = RemoteFunctionData(message=msg) | |
| async def wrapped(input_data: Any) -> Any: | |
| logger = Logger(function_name=path) | |
| ctx = Context(logger=logger) | |
| return await with_context(lambda _: handler(input_data), ctx) | |
| self._functions[path] = RemoteFunctionData(message=msg, handler=wrapped) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/python/iii/src/iii/iii.py` around lines 594 - 596, Remove the
duplicate registration and overwrite that discards the handler: after setting
self._functions[path] = RemoteFunctionData(message=msg, handler=wrapped) and
calling self._send_if_connected(msg), do not assign self._functions[path] =
RemoteFunctionData(message=msg) again; delete that second assignment so the
handler (wrapped) remains stored and the registration message is not sent twice
(references: _functions, RemoteFunctionData, _send_if_connected, wrapped, msg,
path).
- Enhanced readability by adjusting the formatting of the RegisterFunctionMessage instantiation in the III class. - Ensured consistent style across the codebase, aligning with recent refactoring efforts.
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
1 participant
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
registerHttpFunction/register_http_functionfrom Node, Python, and Rust SDKs —registerFunctionnow accepts either a local handler or anHttpInvocationConfigas its second argumentIntoFunctionHandlertrait so both closures andHttpInvocationConfigsatisfy the sameregister_functionsignature; removes the separatehttp_functionsmap andHttpFunctionReftype (renamed toFunctionRef); all functions are now stored in the unifiedfunctionsmap; reconnect re-registers HTTP functions automatically; invocation error now distinguishesfunction_not_invokable(HTTP function) fromfunction_not_foundregisterFunctionoverloaded to acceptRemoteFunctionHandler | HttpInvocationConfig;registerHttpFunctionremoved fromISdkinterface and implementationregister_functionacceptsRemoteFunctionHandler | HttpInvocationConfig;register_http_functionremoved;_http_functionsmap removedtests/http_external_functions.rscovering queue delivery, register/unregister lifecycle, custom headers, multiple functions, stop-after-unregister, and PUT method — mirrors existing Node and Python coverageTest plan
make ci-node— Node integration tests passmake ci-python— Python integration tests passmake ci-rust— Rust integration tests pass (including newhttp_external_functionstests)Summary by CodeRabbit
Breaking Changes
Improvements
New Features
Tests