feat(engine): expose generated HTTP functions as worker groups#1635
Closed
rohitg00 wants to merge 14 commits into
Closed
feat(engine): expose generated HTTP functions as worker groups#1635rohitg00 wants to merge 14 commits into
rohitg00 wants to merge 14 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:
📝 WalkthroughWalkthroughAdds a VirtualWorkerRegistry and crate wiring, propagates a trusted_internal HTTP flag with loopback detection, conditions URL validation on trust, and integrates claim/remove ownership across engine registration, unregister, and cleanup with tests for behavior and rollback. ChangesVirtual Workers and HTTP Trust
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 4
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)
1200-1215:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRoll back
service_registrywhen external registration aborts.
register_service_from_function_idruns before the HTTP module lookup and beforeregister_http_function. Both failure paths return after only releasingexternal_function_owners, so a failed external registration leaves a stale service entry behind even though no function was created.🩹 Suggested rollback
let Some(http_module) = self .service_registry .get_service::<HttpFunctionsWorker>("http_functions") else { + self.service_registry + .remove_function_from_services(®_id); tracing::error!( worker_id = %worker.id, function_id = %reg_id, "HTTP functions module not loaded" ); self.release_external_function_if_owner(&worker.id, ®_id); return Ok(()); }; @@ if let Err(err) = http_module.register_http_function(config).await { tracing::error!( worker_id = %worker.id, function_id = %reg_id, error = ?err, "Failed to register HTTP invocation function" ); + self.service_registry + .remove_function_from_services(®_id); self.release_external_function_if_owner(&worker.id, ®_id); return Ok(()); }Also applies to: 1235-1243
🤖 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 1200 - 1215, The code calls self.service_registry.register_service_from_function_id(®_id) before attempting to look up the HTTP module and before calling register_http_function, so when the lookup or subsequent registration fails the service entry remains stale; update the failure paths (the branch after get_service::<HttpFunctionsWorker>("http_functions") returns None and the similar branch around register_http_function) to roll back the earlier registration by removing/unregistering the service from self.service_registry (e.g. call the appropriate unregister/remove method using reg_id) before calling release_external_function_if_owner and returning, ensuring register_service_from_function_id and register_http_function are symmetrical (register then unregister on any abort).
🤖 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 1186: The code currently derives virtual_worker_name from
Message::RegisterFunction.metadata via take_virtual_worker_name(&mut
reg_metadata), which lets an untrusted worker set metadata.iii.virtualWorker and
thereby flip trusted_internal when invocation.url points to localhost; change
this so the virtual worker flag is not accepted from worker-supplied metadata
but is instead determined from a server-side trust signal tied to the
authenticated worker/session (e.g., authenticated worker identity or session
record), and use that server-verified property when computing trusted_internal;
update the other occurrences (around the block referenced at 1228-1230) to
follow the same pattern so no decision about URL-validation bypass is made from
reg_metadata or any Message::RegisterFunction.metadata.
- Around line 2264-2356: The test
test_external_function_virtual_worker_is_internal never triggers the
loopback/trusted-internal logic because Message::RegisterFunction's
HttpInvocationRef.url uses "http://example.com/…"; update that URL to a loopback
address (e.g., "http://127.0.0.1/…" or "http://localhost/…") so the
trusted-internal branch is exercised, and add an assertion on the virtual
worker's trusted/internal flag (inspect the VirtualWorker struct field used to
record this, e.g., virtual_worker.trusted_internal or similar) after
registration to verify the flag is set true; keep the rest of the test flow
(unregister and cleanup) unchanged.
In `@engine/src/virtual_workers.rs`:
- Around line 1-5: Add the project's standard license header comment block to
the top of engine/src/virtual_workers.rs above the existing use statements
(e.g., above "use std::collections::HashSet;" and the other imports like "use
dashmap::DashMap;", "use serde_json::Value;", "use uuid::Uuid;") so the file
passes the license-header check; ensure the header exactly matches the
repository's canonical header format and encoding.
---
Outside diff comments:
In `@engine/src/engine/mod.rs`:
- Around line 1200-1215: The code calls
self.service_registry.register_service_from_function_id(®_id) before
attempting to look up the HTTP module and before calling register_http_function,
so when the lookup or subsequent registration fails the service entry remains
stale; update the failure paths (the branch after
get_service::<HttpFunctionsWorker>("http_functions") returns None and the
similar branch around register_http_function) to roll back the earlier
registration by removing/unregistering the service from self.service_registry
(e.g. call the appropriate unregister/remove method using reg_id) before calling
release_external_function_if_owner and returning, ensuring
register_service_from_function_id and register_http_function are symmetrical
(register then unregister on any abort).
🪄 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: 4dd3c242-2748-4168-962b-9190eb824cb9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
engine/Cargo.tomlengine/src/engine/mod.rsengine/src/invocation/http_function.rsengine/src/invocation/http_invoker.rsengine/src/lib.rsengine/src/virtual_workers.rsengine/src/workers/engine_fn/mod.rsengine/src/workers/http_functions/mod.rsengine/tests/http_e2e_error_handling.rsengine/tests/http_e2e_security.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 272-279: The is_loopback_http_url function incorrectly checks
host_str() so IPv6 addresses like ::1 (serialized as "[::1]") and other
127.0.0.x addresses are missed; update is_loopback_http_url to parse the URL and
use Url::host() with pattern match on url.host() and call Host::is_loopback()
(or otherwise inspect Host::Ipv4/Ipv6 variants) while still checking
url.scheme() == "http" so all loopback forms are detected; locate the
is_loopback_http_url function and replace the host_str-based logic with
Url::host() + Host::is_loopback().
🪄 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: 31534426-1a63-434a-9abc-6e7f8d321ace
📒 Files selected for processing (5)
engine/src/engine/mod.rsengine/src/virtual_workers.rsengine/src/worker_connections/mod.rsengine/src/workers/worker/rbac_session.rsengine/tests/rbac_infrastructure_e2e.rs
✅ Files skipped from review due to trivial changes (1)
- engine/src/worker_connections/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- engine/src/virtual_workers.rs
rohitg00
changed the title
Contributor
Author
|
Superseded by #1640 after renaming the head branch to generated-http-functions and adding the worker CI cache fix. |
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.