Open
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis PR extends the engine's function handler system to support RBAC session awareness. It introduces a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant Engine
participant ServiceMacro
participant HandlerRegistry
participant InvocationHandler
participant FunctionHandler
participant Session as RBAC Session
Client->>Engine: register_function_handler_with_session(SessionHandler)
Engine->>HandlerRegistry: store SessionHandler with signature
Client->>Engine: invoke_function(input, session_context)
Engine->>InvocationHandler: handle_invocation(input, session)
alt Session Provided
InvocationHandler->>Session: clone session
InvocationHandler->>FunctionHandler: call_handler(invocation_id, input, Some(session))
FunctionHandler->>FunctionHandler: handler(input, Some(session))
else No Session
InvocationHandler->>FunctionHandler: call_handler(invocation_id, input, None)
FunctionHandler->>FunctionHandler: handler(input, None)
end
FunctionHandler-->>Engine: FunctionResult
alt get_functions with Session
Client->>Engine: get_functions(session)
Engine->>HandlerRegistry: filter by RBAC rules
HandlerRegistry-->>Engine: allowed_functions
Engine-->>Client: filtered_function_list
else get_functions without Session
Client->>Engine: get_functions(None)
Engine-->>Client: all_functions
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
engine/src/engine/mod.rs (1)
672-754:⚠️ Potential issue | 🔴 CriticalDon't bypass the normal dispatch path after middleware.
This branch returns before the
match actionlogic and invokesremember_invocation()directly. With middleware enabled,TriggerAction::Enqueuebecomes an immediate invoke,TriggerAction::Voidstill sends anInvocationResult, and the_caller_worker_idinjection fromspawn_invoke_function()is lost. Please feed the enriched payload back through the same post-action dispatch path so middleware doesn't change invocation semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/engine/mod.rs` around lines 672 - 754, The middleware branch is bypassing the normal dispatch path by calling remember_invocation() directly (losing semantics for TriggerAction::Enqueue, TriggerAction::Void and the _caller_worker_id set by spawn_invoke_function()); instead, after obtaining enriched_payload from engine.call(&middleware_id, ...), re-enter the same post-action dispatch logic used elsewhere (the match action branch) so the enriched payload flows through the exact same code path that handles Enqueue/Void/normal invokes and preserves _caller_worker_id; in practice replace the direct call to remember_invocation/send_msg in the tokio::spawn with code that reuses the existing dispatch mechanism (or call the same helper used by spawn_invoke_function) passing enriched_payload, inv_id, function_id, traceparent and baggage so behavior remains identical to non-middleware invocations.
🧹 Nitpick comments (1)
engine/src/function.rs (1)
39-45: Avoid cloning every payload incall_handler.
datais already owned here and isn't reused after dispatch, sodata.clone()adds a full extra JSON copy to every invocation on the hot path.♻️ Proposed fix
pub async fn call_handler( self, invocation_id: Option<Uuid>, data: Value, session: Option<Arc<Session>>, ) -> FunctionResult<Option<Value>, ErrorBody> { - (self.handler)(invocation_id, data.clone(), session).await + (self.handler)(invocation_id, data, session).await }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/function.rs` around lines 39 - 45, call_handler is cloning the owned JSON payload unnecessarily; instead of calling (self.handler)(invocation_id, data.clone(), session).await, pass the owned data directly: (self.handler)(invocation_id, data, session).await. Update this call in the call_handler function (and if the handler signature currently expects a reference, adjust the handler/type to accept Value by value or otherwise move the value) so no extra JSON copy is performed on the hot path.
🤖 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/function-macros/src/lib.rs`:
- Around line 163-183: The helper type_contains_ident is too permissive: before
generating the SessionHandler closure you must validate the function's second
parameter shape exactly (not just that it contains the identifier "Session");
update has_session_param or add an explicit check right before constructing
SessionHandler to accept only Option<::std::sync::Arc<Session>> (or the exact
allowed variants) and otherwise emit a compile_error! with a clear message;
reference the symbols type_contains_ident, has_session_param, and SessionHandler
so the check is placed just before the SessionHandler generation block and
rejects/diagnoses unsupported shapes rather than letting the generated closure
produce opaque type errors.
---
Outside diff comments:
In `@engine/src/engine/mod.rs`:
- Around line 672-754: The middleware branch is bypassing the normal dispatch
path by calling remember_invocation() directly (losing semantics for
TriggerAction::Enqueue, TriggerAction::Void and the _caller_worker_id set by
spawn_invoke_function()); instead, after obtaining enriched_payload from
engine.call(&middleware_id, ...), re-enter the same post-action dispatch logic
used elsewhere (the match action branch) so the enriched payload flows through
the exact same code path that handles Enqueue/Void/normal invokes and preserves
_caller_worker_id; in practice replace the direct call to
remember_invocation/send_msg in the tokio::spawn with code that reuses the
existing dispatch mechanism (or call the same helper used by
spawn_invoke_function) passing enriched_payload, inv_id, function_id,
traceparent and baggage so behavior remains identical to non-middleware
invocations.
---
Nitpick comments:
In `@engine/src/function.rs`:
- Around line 39-45: call_handler is cloning the owned JSON payload
unnecessarily; instead of calling (self.handler)(invocation_id, data.clone(),
session).await, pass the owned data directly: (self.handler)(invocation_id,
data, session).await. Update this call in the call_handler function (and if the
handler signature currently expects a reference, adjust the handler/type to
accept Value by value or otherwise move the value) so no extra JSON copy is
performed on the hot path.
🪄 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: a7bada93-39ae-4846-8dc0-c7964144760d
📒 Files selected for processing (21)
docs/how-to/worker-rbac.mdxengine/function-macros/src/lib.rsengine/src/condition.rsengine/src/engine/mod.rsengine/src/function.rsengine/src/invocation/mod.rsengine/src/modules/bridge_client/mod.rsengine/src/modules/engine_fn/mod.rsengine/src/modules/queue/adapters/builtin/adapter.rsengine/src/modules/queue/queue.rsengine/src/modules/telemetry/mod.rsengine/src/modules/worker/rbac_session.rsengine/tests/common/queue_helpers.rsengine/tests/dlq_redrive_e2e.rsengine/tests/queue_e2e_fanout.rsengine/tests/rabbitmq_queue_integration.rssdk/fixtures/config-test.yamlsdk/packages/node/iii-browser/package.jsonsdk/packages/node/iii/tests/rbac-workers.test.tssdk/packages/python/iii/tests/test_rbac_workers.pysdk/packages/rust/iii/tests/rbac_workers.rs
Comment on lines
+163
to
+183
| fn type_contains_ident(ty: &syn::Type, name: &str) -> bool { | ||
| match ty { | ||
| syn::Type::Path(type_path) => type_path.path.segments.iter().any(|seg| { | ||
| if seg.ident == name { | ||
| return true; | ||
| } | ||
| if let syn::PathArguments::AngleBracketed(args) = &seg.arguments { | ||
| args.args.iter().any(|arg| { | ||
| if let syn::GenericArgument::Type(inner_ty) = arg { | ||
| type_contains_ident(inner_ty, name) | ||
| } else { | ||
| false | ||
| } | ||
| }) | ||
| } else { | ||
| false | ||
| } | ||
| }), | ||
| _ => false, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python <<'PY'
import pathlib, re
pattern = re.compile(
r'#\s*\[\s*function(?:\([^\]]*\))?\s*\]\s*'
r'(?:pub\s+)?async\s+fn\s+([A-Za-z_]\w*)\s*\((.*?)\)',
re.S,
)
for path in pathlib.Path('.').rglob('*.rs'):
text = path.read_text(encoding='utf-8', errors='ignore')
for match in pattern.finditer(text):
signature = " ".join(match.group(2).split())
if "Session" not in signature:
continue
print(f"{path}:{match.group(1)}")
print(f" signature: ({signature})")
PYRepository: iii-hq/iii
Length of output: 183
🏁 Script executed:
cd engine/function-macros && wc -l src/lib.rs && echo "---" && cat -n src/lib.rs | sed -n '150,250p'Repository: iii-hq/iii
Length of output: 4307
🏁 Script executed:
cd engine/function-macros && cat -n src/lib.rs | sed -n '237,330p'Repository: iii-hq/iii
Length of output: 5494
🏁 Script executed:
cd engine/function-macros && cat -n src/lib.rs | sed -n '323,370p'Repository: iii-hq/iii
Length of output: 3054
🏁 Script executed:
cd engine/function-macros && grep -n "has_session_param\|type_contains_ident" src/lib.rsRepository: iii-hq/iii
Length of output: 495
🏁 Script executed:
cd engine && find . -name "*.rs" -type f -exec grep -l "SessionHandler\|register_function_handler_with_session" {} \; | head -10Repository: iii-hq/iii
Length of output: 152
🏁 Script executed:
cd engine && cat -n src/engine/mod.rs | grep -A 10 "fn register_function_handler_with_session" | head -20Repository: iii-hq/iii
Length of output: 871
🏁 Script executed:
cd engine && python3 <<'VERIFY'
import re
# Read the lib.rs file
with open('function-macros/src/lib.rs', 'r') as f:
content = f.read()
# Extract the handler_and_registration block for session case
session_block = re.search(
r'let handler_and_registration = if has_session_param \{(.*?)\} else \{',
content,
re.DOTALL
)
if session_block:
block_text = session_block.group(1)
# Look for the exact type of the session parameter
session_param = re.search(
r'SessionHandler::new\(move \|input: Value, session: ([^|]+)\|',
block_text
)
if session_param:
print("Session parameter type in generated code:")
print(f" {session_param.group(1)}")
# Now check how has_session_param is determined
has_session_check = re.search(
r'let has_session_param = non_self_params\.get\(1\)\.map_or\(false, \|arg\| \{.*?type_contains_ident\(&pat_type\.ty, "([^"]+)"\)',
content,
re.DOTALL
)
if has_session_check:
print("\nDetection mechanism: substring search for identifier")
print(f" Identifier: {has_session_check.group(1)}")
print(" Method: type_contains_ident recursively checks if identifier exists anywhere in type tree")
# Analyze the discrepancy
print("\n⚠️ ISSUE CONFIRMED:")
print("- Macro detects if type tree CONTAINS 'Session' anywhere")
print("- Macro generates closure with hardcoded: Option<::std::sync::Arc<Session>>")
print("- These do NOT match for types like: Session, Arc<Session>, Option<Session>, etc.")
print("- Result: Opaque type mismatch errors during compilation")
VERIFYRepository: iii-hq/iii
Length of output: 592
Validate second parameter type exactly before generating SessionHandler.
The type_contains_ident function (lines 163–183) recursively searches for "Session" anywhere in the type tree, causing has_session_param (lines 237–243) to trigger for any second parameter containing that identifier. However, the generated SessionHandler closure (line 326) hardcodes the parameter as Option<::std::sync::Arc<Session>>. This mismatch means signatures like Session, Arc<Session>, or Option<Session> will be accepted by the detection but rejected by the generated code with opaque type errors instead of clear validation.
Add an exact-type check before line 323, or emit a compile_error! for unsupported shapes.
Also applies to: lines 237–243, 323–361
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/function-macros/src/lib.rs` around lines 163 - 183, The helper
type_contains_ident is too permissive: before generating the SessionHandler
closure you must validate the function's second parameter shape exactly (not
just that it contains the identifier "Session"); update has_session_param or add
an explicit check right before constructing SessionHandler to accept only
Option<::std::sync::Arc<Session>> (or the exact allowed variants) and otherwise
emit a compile_error! with a clear message; reference the symbols
type_contains_ident, has_session_param, and SessionHandler so the check is
placed just before the SessionHandler generation block and rejects/diagnoses
unsupported shapes rather than letting the generated closure produce opaque type
errors.
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)
engine/src/engine/mod.rs (1)
1303-1314:⚠️ Potential issue | 🟠 Major
Engine::call()still strips the active session.Line 1313 hardcodes
None, so everyself.call(...)insiderouter_msg()—notably middleware and the RBAC registration hooks—invokes session-aware handlers without the worker's session. That meansregister_function_handler_with_sessioncannot actually see fields likeallowed_functionsorfunction_registration_prefixon those paths. Please add a session-aware internal call path and use it from the worker-session branches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/engine/mod.rs` around lines 1303 - 1314, Engine::call currently passes None for the session into self.invocations.handle_invocation (see invocations.handle_invocation(..., None, ...)), which strips the active worker session for all router_msg paths; add a session-aware internal call path that forwards the active session (the worker/session variable used in the worker-session branches) into handle_invocation and update Engine::call/router_msg to use that path for middleware, RBAC hooks and register_function_handler_with_session so those handlers can read allowed_functions and function_registration_prefix from the session.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@engine/src/engine/mod.rs`:
- Around line 1303-1314: Engine::call currently passes None for the session into
self.invocations.handle_invocation (see invocations.handle_invocation(..., None,
...)), which strips the active worker session for all router_msg paths; add a
session-aware internal call path that forwards the active session (the
worker/session variable used in the worker-session branches) into
handle_invocation and update Engine::call/router_msg to use that path for
middleware, RBAC hooks and register_function_handler_with_session so those
handlers can read allowed_functions and function_registration_prefix from the
session.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 480dd6c2-2634-4901-941d-521db4cf3e1a
📒 Files selected for processing (4)
engine/src/engine/mod.rssdk/packages/node/iii/tests/rbac-workers.test.tssdk/packages/python/iii/tests/test_rbac_workers.pysdk/packages/rust/iii/tests/rbac_workers.rs
✅ Files skipped from review due to trivial changes (1)
- sdk/packages/python/iii/tests/test_rbac_workers.py
🚧 Files skipped from review as they are similar to previous changes (1)
- sdk/packages/node/iii/tests/rbac-workers.test.ts
guibeira
reviewed
Apr 1, 2026
| engine.send_msg(&w, response).await; | ||
| }); | ||
| return Ok(()); | ||
| if !function_id.starts_with("engine::") { |
Contributor
There was a problem hiding this comment.
Should we include a logger message saying this prefix is not allowed?
| request: RegisterFunctionRequest, | ||
| handler: SessionHandler<H>, | ||
| ) where | ||
| H: Fn(Value, Option<Arc<Session>>) -> F + Send + Sync + 'static, |
Contributor
There was a problem hiding this comment.
I noticed we use this types in many places. Can we create a type name like this:
type JsonHandler<F> = dyn Fn(Value, Option<Arc<Session>>) -> F + Send + Sync + 'static;same as the other one
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 by CodeRabbit
Release Notes
New Features
Tests
Chores