feat(lfx): add capability routing pluggability - #13617
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (54.73%) is below the target coverage (60.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## feat/executor-seam-stepflow-mvp #13617 +/- ##
===================================================================
- Coverage 58.84% 58.67% -0.18%
===================================================================
Files 2320 2317 -3
Lines 221132 219749 -1383
Branches 34351 32992 -1359
===================================================================
- Hits 130121 128932 -1189
+ Misses 89494 89349 -145
+ Partials 1517 1468 -49
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
erichare
left a comment
There was a problem hiding this comment.
Findings
-
[P1] Reserved capability metadata can be spoofed when
mint()returns no token.
src/lfx/src/lfx/execution/coordinator.py:49-54merges caller/runtime options withdecision.runtime_options, butsrc/lfx/src/lfx/services/capability/service.py:115-120only writeslfx_capability_tokenwhen the provider returns a truthy token. A caller-suppliedlfx_capability_tokencan therefore survive into the selected executor. Strip reservedlfx_*capability keys before partitioning/merging, or make the routing decision the sole source of those fields and add a test where an active provider returnsNone. -
[P2] Untrusted classification falls back to the default executor when no untrusted executor is configured.
src/lfx/src/lfx/services/capability/service.py:103-106leavesexecutor_kindas the default ifTrust.UNTRUSTEDis returned but_untrusted_executor_kindisNone. That is a risky fail-open mode for an isolation/routing boundary. I’d fail closed with a clear error unless there is an explicit “allow untrusted on default” opt-in, and cover that misconfiguration with a unit test.
fabcc03 to
8890e9d
Compare
f9cc37e to
2789ac0
Compare
✅ Test Coverage AdvisorNo source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉
|
8890e9d to
7ab1e8b
Compare
2789ac0 to
d82f246
Compare
erichare
left a comment
There was a problem hiding this comment.
Hi @ogabrielluiz this looks great, just two things:
[P1] Active capability routing breaks Graph.arun() on the in-process legacy path.
src/lfx/src/lfx/services/capability/service.py:121-123 adds lfx_tenant_id / lfx_trust to every active routing decision, even when the decision keeps the default in-process executor. The in-process legacy path then forwards every non-underscore runtime option into _arun_legacy at src/lfx/src/lfx/execution/backends/in_process.py:24-26, but Graph._arun_legacy does not accept those new keywords (src/lfx/src/lfx/graph/graph/base.py:897-908). So enabling any non-passthrough capability provider can make existing Graph.arun() calls fail with TypeError: ... unexpected keyword argument 'lfx_tenant_id'. I reproduced this with a minimal PR-head snippet.
Suggested fix: filter capability-only keys before calling _arun_legacy (including lfx_capability_token, lfx_tenant_id, lfx_trust, and probably request-only scope keys), and add a regression using real InProcessExecutor + active CapabilityService + _use_arun_legacy=True.
[P2] Custom primitives that subclass the defaults are silently treated as passthrough.
CapabilityService.is_passthrough uses isinstance(...) for NoopCapabilityProvider, AllTrustedClassifier, and SingleTenantResolver at src/lfx/src/lfx/services/capability/service.py:76-80. If a plugin subclasses one of those defaults to reuse behavior while overriding mint() or trust classification, the coordinator sees is_passthrough == True and skips routing entirely at src/lfx/src/lfx/execution/coordinator.py:83.
Suggested fix: use exact type checks or maintain an explicit “custom primitive installed” flag in install().
d82f246 to
dfb455b
Compare
erichare
left a comment
There was a problem hiding this comment.
Thanks for the updates @ogabrielluiz — the two findings from my first pass are resolved:
- ✅ Spoofed capability metadata —
run()strips reserved keys up front (coordinator.py:81) and re-introduces them only fromdecision.runtime_options(coordinator.py:98-99); the "provider returns no token" case has a test. - ✅ Untrusted fail-open —
service.py:115-117now raises whenTrust.UNTRUSTEDhas nountrusted_executor_kindconfigured, with a fail-closed test.
Two items from the second round are still open:
[P1] lfx_tenant_id / lfx_trust still break Graph.arun()'s legacy path.
route() injects both keys whenever not is_passthrough (service.py:121-123), even when the decision keeps the default in-process executor (e.g. a trusted flow under a custom resolver). The coordinator merges decision.runtime_options back into the unit (coordinator.py:98-99), and InProcessExecutor forwards every non-_ key into graph._arun_legacy(**legacy_kwargs) (in_process.py:25-26). _arun_legacy has no **kwargs (graph/graph/base.py:897), so the legacy path raises TypeError: unexpected keyword argument 'lfx_tenant_id'. Please strip the reserved lfx_* keys in in_process.py before the legacy call, and add a regression with a real InProcessExecutor + active CapabilityService + _use_arun_legacy=True.
[P2] Subclassed defaults are silently treated as passthrough.
is_passthrough uses isinstance(...) (service.py:77-79), so a plugin that subclasses NoopCapabilityProvider to override only mint() reads as passthrough and route() is never called (coordinator.py:83). Use exact type(x) is ... checks, or an explicit "custom primitive installed" flag set in install().
Nit: get_capability_service() in deps.py is missing the return annotation its siblings carry.
43a682b to
ea33e98
Compare
05b36b3 to
cf5ffa1
Compare
Coordinator.stream() no longer takes an inputs parameter (the streaming path reads initial_inputs from runtime_options); update the capability routing stream-close test to the current signature.
ExecutorService declares a hard dependency on CAPABILITY_SERVICE, but the langflow backend's register_all_service_factories() registered ExecutorServiceFactory without CapabilityServiceFactory. The service manager could not resolve the dependency when building ExecutorService, so every flow run failed with NoFactoryRegisteredError / "ExecutorService is not available". lfx's own bootstrap (get_factories) auto-discovers the capability factory, so lfx run/serve were unaffected; only the backend's hand-maintained factory list had drifted. Add a regression test asserting the backend registers a factory for every ExecutorService dependency.
ea33e98 to
1340f30
Compare
cf5ffa1 to
76e0398
Compare
Summary
Stacks on #13063. Adds the Langflow-side capability-routing contract needed for pluggable execution policy.
This keeps the scope intentionally narrow: Langflow can ask a pluggable capability provider how to route a run and which opaque per-run metadata to attach, while the provider implementation remains outside this PR.
What changed
lfx.services.capability:CapabilityServiceServiceType.CAPABILITY_SERVICEand service dependency wiring.ExecutorServicecreate aCoordinatorwith a capability service.Coordinatorto request a routing decision before dispatch.Unit.executor_kindso routing can select a non-default executor per unit.Scope boundary
This PR is pluggability only. It defines the contract and default noop behavior, but it does not ship a concrete provider implementation.
The default provider is noop, so behavior remains unchanged unless a capability provider is installed/configured.
How this fits the stack
Verification
src/lfx/tests/unit/services/capability/test_capability_service.pysrc/lfx/tests/unit/execution/test_capability_routing.pyReview focus