feat(engine): expose generated HTTP functions as worker groups#1640
Open
rohitg00 wants to merge 34 commits into
Open
feat(engine): expose generated HTTP functions as worker groups#1640rohitg00 wants to merge 34 commits into
rohitg00 wants to merge 34 commits into
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
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:
📝 WalkthroughWalkthroughAdds a generated-worker registry and trust gating for generated HTTP function registration, integrates claim/rollback ownership into engine registration/unregistration, skips URL validation for trusted internal endpoints, updates worker discovery, and adds e2e/sdk tests and CI/dependency tweaks. ChangesGenerated Worker Registration and Trust Gating
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
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/engine/mod.rs (1)
1231-1243:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFailed external re-registration can orphan an existing live function.
These rollback paths always drop
external_function_owners, even when the samefunction_idalready had a valid registration before this attempt. If the new registration fails before replacing the handler, the oldengine.functions/http_functionsentry stays installed but loses its owner, socleanup_workerwill later skip teardown and leave a stale function behind.Also applies to: 1263-1272
🤖 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 `@engine/src/engine/mod.rs` around lines 1231 - 1243, The rollback branch in the re-registration path drops ownership unconditionally, which can orphan an existing live function: ensure that when a new registration attempt fails (e.g., in the branches around service_registry.get_service::<HttpFunctionsWorker>("http_functions") and the other similar branch), you only remove or release ownership if this worker actually became the owner; change the logic around external_function_owners and the call to release_external_function_if_owner(&worker.id, ®_id) to check current owner or previous registration state before removing it, and avoid clearing external_function_owners or calling release_external_function_if_owner when an earlier valid registration (engine.functions / http_functions) remains installed so cleanup_worker can still teardown correctly.
🤖 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.
Inline comments:
In `@engine/src/engine/mod.rs`:
- Around line 1212-1215: The registry and HTTP config are using different trust
logic: compute the trust predicate once (e.g., use the result of
trusted_generated_worker_name(..., &mut reg_metadata) or an explicit
is_trusted_generated_worker boolean based on the same session+loopback checks)
and reuse that single boolean for both constructing
HttpFunctionConfig.trusted_internal and for
generated_workers.claim_function(..., trusted_generated_worker ...); update the
code around trusted_generated_worker_name / reg_metadata and the
HttpFunctionConfig creation so they read the same variable (remove any
duplicated trust checks at the HttpFunctionConfig site) and ensure the same
boolean is passed into generated_workers.claim_function to keep registry and
HTTP config consistent.
In `@engine/src/generated_workers.rs`:
- Around line 124-126: The code currently uses
iii_object.remove("generatedWorker").or_else(||
iii_object.remove("virtualWorker"))? which only removes one of the two private
hint keys; update the logic so both private hint keys ("generatedWorker" and
"virtualWorker") are removed from iii_object regardless of order, then select
the alias from whichever value existed (prefer "generatedWorker" if present,
otherwise fall back to "virtualWorker"); ensure the variable hint still holds
the chosen value and that both keys are cleared from iii_object to avoid leaking
private hints into public metadata.
In `@engine/src/workers/engine_fn/mod.rs`:
- Line 351: The generated-worker entries currently recompute connected_at_ms on
each engine::workers::list call using chrono::Utc::now(), which misrepresents
actual registration time; add a timestamp field (e.g., registered_at: i64) to
the GeneratedWorkerInfo struct, set that field once when the worker is created
in claim_function, and then change the list serialization to use
generated_worker.registered_at as u64 (instead of
chrono::Utc::now().timestamp_millis()) so the reported connected_at_ms reflects
the original registration time.
---
Outside diff comments:
In `@engine/src/engine/mod.rs`:
- Around line 1231-1243: The rollback branch in the re-registration path drops
ownership unconditionally, which can orphan an existing live function: ensure
that when a new registration attempt fails (e.g., in the branches around
service_registry.get_service::<HttpFunctionsWorker>("http_functions") and the
other similar branch), you only remove or release ownership if this worker
actually became the owner; change the logic around external_function_owners and
the call to release_external_function_if_owner(&worker.id, ®_id) to check
current owner or previous registration state before removing it, and avoid
clearing external_function_owners or calling release_external_function_if_owner
when an earlier valid registration (engine.functions / http_functions) remains
installed so cleanup_worker can still teardown correctly.
🪄 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: a2e08645-a255-40fc-af31-cdfbab57371d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.github/workflows/ci.ymlengine/Cargo.tomlengine/src/engine/mod.rsengine/src/generated_workers.rsengine/src/invocation/http_function.rsengine/src/invocation/http_invoker.rsengine/src/lib.rsengine/src/worker_connections/mod.rsengine/src/workers/engine_fn/mod.rsengine/src/workers/http_functions/mod.rsengine/src/workers/worker/rbac_session.rsengine/tests/generated_worker_bridge_e2e.rsengine/tests/http_e2e_error_handling.rsengine/tests/http_e2e_security.rsengine/tests/rbac_infrastructure_e2e.rssdk/packages/node/iii/tests/http-external-functions.test.tssdk/packages/python/iii/tests/test_http_external_functions_integration.pysdk/packages/rust/iii/tests/http_external_functions.rs
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)
1104-1127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep external unregister teardown symmetric with the cleanup path.
If
unregister_http_function()fails here, we only log and continue after already clearinggenerated_workers.cleanup_worker()handles the same failure by callingremove_function(...), so this branch can leave an orphanedengine.functionsentry behind while discovery state is gone.Suggested fix
- self.generated_workers.remove_function(&resolved_id); if let Some(http_module) = self .service_registry .get_service::<HttpFunctionsWorker>("http_functions") @@ Err(err) => { tracing::error!( worker_id = %worker.id, function_id = %id, error = ?err, "Failed to unregister external function" ); + self.remove_function(&resolved_id); } } + self.generated_workers.remove_function(&resolved_id); self.service_registry .remove_function_from_services(&resolved_id);🤖 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 `@engine/src/engine/mod.rs` around lines 1104 - 1127, The branch that handles a failed unregister call leaves engine state inconsistent because self.generated_workers.remove_function(&resolved_id) ran before attempting http_module.unregister_http_function(&resolved_id). On the Err path, mirror cleanup_worker by invoking the same engine-level removal used there (call the engine's remove_function(&resolved_id) or equivalent method that cleanup_worker uses) so the engine.functions entry is cleared when unregister_http_function fails; keep the existing service_registry.remove_function_from_services(&resolved_id) usage as appropriate after ensuring engine-level removal.
♻️ Duplicate comments (1)
engine/src/engine/mod.rs (1)
1320-1325:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIgnore
generatedWorkerhints on non-HTTP registrations.
generated_worker_nameonly proves the session is trusted. This branch has no loopback URL to validate, but it still records the function as a trusted generated worker, so any trusted/local WS worker can spoof a generated worker group via metadata alone.Suggested fix
- if let Some(worker_name) = generated_worker_name { - self.generated_workers - .claim_function(worker_name, worker.id, true, ®_id); - } else { - self.generated_workers.remove_function(®_id); - } + self.generated_workers.remove_function(®_id);🤖 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 `@engine/src/engine/mod.rs` around lines 1320 - 1325, The code currently treats any registration with generated_worker_name as trusted; update the branch handling generated_worker_name so it only calls self.generated_workers.claim_function(...) when the registration is an HTTP registration with a valid loopback URL (i.e., the registration type/metadata proves a loopback endpoint), otherwise call self.generated_workers.remove_function(®_id). Locate the branch using generated_worker_name, self.generated_workers.claim_function(..., worker.id, true, ®_id), and self.generated_workers.remove_function(®_id) and add a guard that verifies the registration has an HTTP loopback URL (or equivalent trusted indicator) before claiming the function.
🤖 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.
Inline comments:
In `@engine/src/engine/mod.rs`:
- Line 3243: The seeded function IDs use a dot separator ("external.invalid_url"
and "external.cleanup") instead of the required "::" form; change the string
literals id: "external.invalid_url".to_string() and the other occurrence at the
later location to id: "external::invalid_url".to_string() (and
"external::cleanup") and update any tests/assertions or setup code that match
the old IDs to the new "external::..." values so all comparisons and registry
lookups use the "::" separator.
---
Outside diff comments:
In `@engine/src/engine/mod.rs`:
- Around line 1104-1127: The branch that handles a failed unregister call leaves
engine state inconsistent because
self.generated_workers.remove_function(&resolved_id) ran before attempting
http_module.unregister_http_function(&resolved_id). On the Err path, mirror
cleanup_worker by invoking the same engine-level removal used there (call the
engine's remove_function(&resolved_id) or equivalent method that cleanup_worker
uses) so the engine.functions entry is cleared when unregister_http_function
fails; keep the existing
service_registry.remove_function_from_services(&resolved_id) usage as
appropriate after ensuring engine-level removal.
---
Duplicate comments:
In `@engine/src/engine/mod.rs`:
- Around line 1320-1325: The code currently treats any registration with
generated_worker_name as trusted; update the branch handling
generated_worker_name so it only calls
self.generated_workers.claim_function(...) when the registration is an HTTP
registration with a valid loopback URL (i.e., the registration type/metadata
proves a loopback endpoint), otherwise call
self.generated_workers.remove_function(®_id). Locate the branch using
generated_worker_name, self.generated_workers.claim_function(..., worker.id,
true, ®_id), and self.generated_workers.remove_function(®_id) and add a
guard that verifies the registration has an HTTP loopback URL (or equivalent
trusted indicator) before claiming the function.
🪄 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: 543d4793-7026-4767-b3da-eea2cb004915
📒 Files selected for processing (3)
engine/src/engine/mod.rsengine/src/generated_workers.rsengine/src/workers/engine_fn/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- engine/src/workers/engine_fn/mod.rs
- engine/src/generated_workers.rs
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/workers/http_functions/mod.rs (1)
90-104:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd a doc comment to
register_http_functionto document thetrusted_internalprivilege.The only production code path setting
trusted_internal: trueis inengine/src/engine/mod.rs:1259–1260, which correctly enforces both loopback validation and session trust before callingregister_http_function. However, since the method ispubon a service-discoverable worker, a doc comment should explicitly document thattrusted_internal: truemust only be set after upstream loopback + session trust gates have been confirmed. This prevents future refactors or callers from accidentally bypassing validation.🤖 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 `@engine/src/workers/http_functions/mod.rs` around lines 90 - 104, Add a doc comment above the public async function register_http_function documenting the meaning and safety requirements of the trusted_internal flag: state that trusted_internal == true grants bypass of URL validation and must only be set by callers that have already enforced loopback/host-only checks and session trust (i.e., the upstream gate that currently sets trusted_internal must be retained), warn that this function is exposed by a service-discoverable worker and that callers must not set trusted_internal to true without those checks, and include guidance for future maintainers to audit callers before allowing trusted_internal=true.
🤖 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.
Inline comments:
In `@engine/src/workers/http_functions/mod.rs`:
- Around line 127-137: Add two unit tests to lock behavior: (1) a test that
calls unregister_http_function("does.not.exist") and asserts it returns an Err
with code "function_not_found" to cover the not-found branch in
unregister_http_function; (2) a test that builds a FunctionConfig with
trusted_internal = true and an invalid URL (e.g. "://bad-url"), calls
register_http_function(config) and asserts it succeeds and that
module.http_functions contains the function key, verifying that
register_http_function bypasses URL validation when trusted_internal is set. Use
existing helpers like build_module, make_function_config and
ensure_default_meter to set up the tests.
---
Outside diff comments:
In `@engine/src/workers/http_functions/mod.rs`:
- Around line 90-104: Add a doc comment above the public async function
register_http_function documenting the meaning and safety requirements of the
trusted_internal flag: state that trusted_internal == true grants bypass of URL
validation and must only be set by callers that have already enforced
loopback/host-only checks and session trust (i.e., the upstream gate that
currently sets trusted_internal must be retained), warn that this function is
exposed by a service-discoverable worker and that callers must not set
trusted_internal to true without those checks, and include guidance for future
maintainers to audit callers before allowing trusted_internal=true.
🪄 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: b71b6592-ff9f-4428-9125-825eeb164638
📒 Files selected for processing (2)
engine/src/engine/mod.rsengine/src/workers/http_functions/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- engine/src/engine/mod.rs
This reverts commit c4e0900.
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
engine::workers::listmetadata.iii.generatedWorkerhint before public function metadata is storedmetadata.iii.virtualWorkerkey as a compatibility aliasiii-http-functionsby default so converted OpenAPI/MCP calls can register without extra configValidation
cargo fmt --all -- --checkcargo test -p iii --test generated_worker_bridge_e2ecargo test -p iii generated_worker --libcargo test -p iii test_list_worker_infos_includes_generated_group_without_private_public_shape --libcargo test -p iii http_functions --libcargo check -p iii --testscargo test -p iii --test http_e2e_securitycargo test -p iii --test http_e2e_error_handlingcargo fmt -p iii-sdk -- --checkpnpm install --filter iii-sdk --frozen-lockfilepnpm --filter iii-sdk exec tsc --noEmitpython3 -m compileall -q sdk/packages/python/iii/src sdk/packages/python/iii/tests/test_http_external_functions_integration.pycargo test -p iii-sdk --test http_external_functions exposes_generated_http_functions --no-runcargo fmt,cargo test,cargo clippy --all-targets --all-features -- -D warningsSpec-to-Worker E2E Contract
The paired worker PR is iii-experimental/spec-to-worker#13. It now uses generic fixtures and docs instead of Context7-specific examples. The expected contract is:
engine::workers::listshows the generated source as a normal engine-runtime worker groupNotes
spec-to-worker::convertregisters normal iii functions backed by HTTP invocation. The engine exposes a user-facing worker grouping for discovery, while routing remains engine-owned and no backing worker process is started for that group. Other workers see ordinary worker/function shapes, not private generated routing details.Summary by CodeRabbit
New Features
Improvements
Tests
Chores