feat: initial Rust Core (cpex-core and cpex-sdk)#13
Conversation
Signed-off-by: Teryl Taylor <terylt@ibm.com>
araujof
left a comment
There was a problem hiding this comment.
Nice work! Here are a few findings to address.
TL;DR
Scope and shape are right. The trait decomposition (Plugin lifecycle + HookHandler<H> dispatch + PluginRef trusted config) looks good. But some pieces look like stubs, and a few items are behavioral bugs rather than future-phase gaps. The biggest issues: the "concurrent" phase is serial, timeouts don't actually time anything out, OnError::Disable is silently equivalent to Ignore, and register_legacy never wires plugins into the hook index.
Design
Good
- Trust model is right.
PluginRef.trusted_configas the sole source of scheduling; plugin cannot tamper. Covered by a dedicated test. - Split of payload vs extensions matches the CMF invariant story and avoids COW on label-only changes.
HookHandler<H>borrow-based signature lets the framework retain ownership; plugins pay clone cost only when modifying. Enforced by the borrow checker.- Open
HookTypenewtype keeps hosts extensible while giving compile-time constants for built-ins.
Concerns
AnyHookHandler::invokeis sync whilePlugin::initialize/shutdownareasync_trait. A real plugin doing HTTP / DB lookups has no async entry point in the hot path. This will force eitherblock_on(deadlock risk inside a tokio runtime) or a re-plumb when Phase 5 (PyO3) and Phase 7 (wasmtime) land, both inherently async. Decide now whetherinvokereturnsPin<Box<dyn Future>>or stays sync; retrofitting later breaks every binding.define_hook!generates an orphan trait. The macro emitsMyHookHandlerwith an owned payload parameter, but the executor dispatches only through the genericHookHandler<H>(borrowed). The generated trait is never implemented or invoked anywhere.config.rsis aTODOcomment. Either ship a minimalserde_yamlround-trip now or dropserde_yamlfromcpex-core's deps.ManagerConfigwraps a single field. Today it's justExecutorConfig. If no second field is coming in future phases, flatten it.- Stale docs.
registry.rsdocuments a removedinvoke_owned/invoke_refsplit.define_hook!docs claim "owned; executor decides borrow vs clone" but the executor never passes owned payloads anywhere.
Bugs
| # | Severity | Location | Issue |
|---|---|---|---|
| 1 | High | executor.rs:350–421 |
run_concurrent_phase runs plugins serially — the loop for entry in entries { … .await } awaits each in turn. No join_all / FuturesUnordered / tokio::spawn. |
| 2 | High | executor.rs:246–249, 327–330, 372–375 |
timeout(dur, async { entry.handler.invoke(...) }) wraps a synchronous call. The inner future returns immediately, so the timeout is never observed. A misbehaving plugin can hang the pipeline indefinitely. Either make invoke async or run the sync call via tokio::task::spawn_blocking under the timeout. |
| 3 | High | executor.rs:283, 297 |
OnError::Disable is matched together with OnError::Ignore and has no effect; the plugin is never marked disabled. A plugin configured to auto-disable after failure will keep running and keep failing. |
| 4 | High | registry.rs:262–276 |
register_legacy inserts into plugins but never into hook_index. Every plugin registered this way is silently unreachable. No test covers it. Either remove the method or wire it up. |
| 5 | Medium | executor.rs:204–211 |
FireAndForget runs inline in run_ref_phase, blocking the pipeline. The design promises "background tokio tasks." Use tokio::spawn and don't await. |
| 6 | Medium | executor.rs:455–457 |
extract_erased returns Option<_> and silently drops on downcast failure. A handler returning the wrong result type causes the pipeline to continue as if the plugin did nothing; no log, no error. At minimum warn! on None. |
| 7 | Medium | executor.rs:266–269 |
modify_extensions replaces the whole Extensions struct (*extensions = me). A Transform plugin can clear labels, directly violating the monotonic-label invariant the design doc treats as type-system-guaranteed. Mark as a hard TODO in code, or enforce a superset check. |
| 8 | Medium | executor.rs:242 |
FilteredExtensions::default() is hard-coded for every dispatch. Plugins see no extensions regardless of declared capabilities. If a plugin relies on reading labels today it will always see empty, and a future fix that starts populating them will be a silent behavior change. |
| 9 | Low | manager.rs:245–252 |
initialize() returns on first plugin failure, leaving partially-initialized state. No shutdown() of already-initialized plugins before returning the error. |
| 10 | Low | error.rs:95 + executor |
PluginViolation.plugin_name is documented as "set by the framework", but the executor never stamps it. Callers can't attribute a violation to its source. |
Code quality
unsafe impl Send + SynconTypedHandlerAdapter(adapter.rs) is unnecessary.Arc<P>whereP: Send+Syncis auto-Send+Sync;PhantomData<H>withH: Send+Syncis as well. Dropping theunsafelets the compiler catch a future bound regression automatically.- Dead field
PluginManager.config(compiler warning). Clone out what's needed at construction or drop the field. fn default_priority() -> i32 { 100 }→const.has_hooks_forin registry re-checks!v.is_empty()even thoughunregisteralreadyretains empty entries out.initialize/shutdowniterateplugin_names()thenget(name): two lookups per plugin. CollectPluginRefclones once and iterate.register_handler+register_handler_for_namesduplicate the adapter construction. Extract a helper.impl Default for ManagerConfigcan be#[derive(Default)].
Security
- CMF invariants not enforced. That's fine for Phase 1a scope, but the
modify_extensionsreplace path in the executor means plugins can erase labels today, inheriting weaker guarantees than the design advertises. Maybe document this in the issue/README, or gatemodify_extensionsbehind a#[cfg(phase_3)]until the tiered types land. - No capability filtering (bug
#8). Label-sensitive plugins written now will be wrong when Phase 3 turns filtering on. - Timeout is decorative (bug
#2). A blocking sync handler, or a plugin that deliberately spins, stalls the whole pipeline. This is exploitable with a malicious or buggy plugin. OnError::Disableunenforced (bug#3). A failing plugin configured asdisablecontinues to run on every invocation, both wasting work and potentially amplifying a failure mode.- Violation attribution missing (bug
#10). Audit logs can't reliably identify which plugin denied a request. - No payload-size or hook-depth limits. Acceptable for Phase 1a but problematic for Phase 1b/2 when FFI adds an untrusted ingress.
Tests
Tests cover registration, priority ordering, trusted-config tamper protection, allow/deny/modify, lifecycle, and typed vs dynamic invoke. Gaps worth closing before 1b:
- No test for
PluginMode::Transformactually transforming. - No test for
Concurrentwith >1 plugin (would expose bug#1if asserting concurrency via timing). - No test for timeout firing (would expose bug
#2). - No test for
OnError::Disablesemantics (would expose bug#3). - No test for
register_legacydispatch (would expose bug#4). - No test for
FireAndForgetreturning before the background task finishes (would expose bug#5). - No test for
modify_extensionssuperset/monotonic behavior. - No test asserting extensions are capability-filtered.
Recommendation
Request changes. High-severity items (concurrent serial, fake timeout, silent Disable, orphan register_legacy, blocking FireAndForget). Either fix them in this PR or document them out explicitly in the issue/epic with a "not functional in 1a" banner. Once the Highs are addressed, the rest can land as follow-up commits in Phase 1b.
Signed-off-by: Teryl Taylor <terylt@ibm.com>
|
All review items addressed or described where they will be deferred. Async invoke (was sync) Concurrent phase was serial OnError::Disable did nothing register_legacy dead end FireAndForget ran inline extract_erased silently dropped modify_extensions replaces whole struct FilteredExtensions::default() hardcoded initialize() partial failure PluginViolation.plugin_name never stamped Cleanup:
Context refactor (spec alignment):
New tests (34 total, up from 27):
Context state semantics by phase: Handlers receive
The Two new tests verify this: |
araujof
left a comment
There was a problem hiding this comment.
LGTM
Nice work addressing the reviews.
* feat: initial revision rust core. Signed-off-by: Teryl Taylor <terylt@ibm.com> * fix: addressed comments in PR. Updated PluginContext to match spec. Signed-off-by: Teryl Taylor <terylt@ibm.com> --------- Signed-off-by: Teryl Taylor <terylt@ibm.com> Co-authored-by: Teryl Taylor <terylt@ibm.com>
* feat: initial revision rust core. Signed-off-by: Teryl Taylor <terylt@ibm.com> * fix: addressed comments in PR. Updated PluginContext to match spec. Signed-off-by: Teryl Taylor <terylt@ibm.com> --------- Signed-off-by: Teryl Taylor <terylt@ibm.com> Co-authored-by: Teryl Taylor <terylt@ibm.com>
* feat: initial revision rust core. Signed-off-by: Teryl Taylor <terylt@ibm.com> * fix: addressed comments in PR. Updated PluginContext to match spec. Signed-off-by: Teryl Taylor <terylt@ibm.com> --------- Signed-off-by: Teryl Taylor <terylt@ibm.com> Co-authored-by: Teryl Taylor <terylt@ibm.com>
* feat: initial revision rust core. Signed-off-by: Teryl Taylor <terylt@ibm.com> * fix: addressed comments in PR. Updated PluginContext to match spec. Signed-off-by: Teryl Taylor <terylt@ibm.com> --------- Signed-off-by: Teryl Taylor <terylt@ibm.com> Co-authored-by: Teryl Taylor <terylt@ibm.com>
* feat: initial revision rust core. Signed-off-by: Teryl Taylor <terylt@ibm.com> * fix: addressed comments in PR. Updated PluginContext to match spec. Signed-off-by: Teryl Taylor <terylt@ibm.com> --------- Signed-off-by: Teryl Taylor <terylt@ibm.com> Co-authored-by: Teryl Taylor <terylt@ibm.com>
* feat: initial revision rust core. Signed-off-by: Teryl Taylor <terylt@ibm.com> * fix: addressed comments in PR. Updated PluginContext to match spec. Signed-off-by: Teryl Taylor <terylt@ibm.com> --------- Signed-off-by: Teryl Taylor <terylt@ibm.com> Co-authored-by: Teryl Taylor <terylt@ibm.com>
* feat: initial Rust Core (cpex-core and cpex-sdk) (#13) * feat: initial revision rust core. Signed-off-by: Teryl Taylor <terylt@ibm.com> * fix: addressed comments in PR. Updated PluginContext to match spec. Signed-off-by: Teryl Taylor <terylt@ibm.com> --------- Signed-off-by: Teryl Taylor <terylt@ibm.com> Co-authored-by: Teryl Taylor <terylt@ibm.com> * feat: CPEX Rust config (#38) * feat: added yaml and routing rule support. Signed-off-by: Teryl Taylor <terylt@ibm.com> * feat: added example code to show how to load manager and plugins. Signed-off-by: Teryl Taylor <terylt@ibm.com> * fixes: updated plugin errors, configs to more match python. Signed-off-by: Teryl Taylor <terylt@ibm.com> --------- Signed-off-by: Teryl Taylor <terylt@ibm.com> Co-authored-by: Teryl Taylor <terylt@ibm.com> * feat: RUST with CMF and extensions. (#44) * feat: initial revision rust core. Signed-off-by: Teryl Taylor <terylt@ibm.com> * fix: addressed comments in PR. Updated PluginContext to match spec. Signed-off-by: Teryl Taylor <terylt@ibm.com> * feat: added yaml and routing rule support. Signed-off-by: Teryl Taylor <terylt@ibm.com> * feat: added example code to show how to load manager and plugins. Signed-off-by: Teryl Taylor <terylt@ibm.com> * fixes: updated plugin errors, configs to more match python. Signed-off-by: Teryl Taylor <terylt@ibm.com> * feat: RUST CMF initial revision. Signed-off-by: Teryl Taylor <terylt@ibm.com> * feat: added invoke named support, added constants, fixed reviewed code. Signed-off-by: Teryl Taylor <terylt@ibm.com> * feat: added owned extensions and did some refactoring. Signed-off-by: Teryl Taylor <terylt@ibm.com> --------- Signed-off-by: Teryl Taylor <terylt@ibm.com> Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com> Co-authored-by: Teryl Taylor <terylt@ibm.com> Co-authored-by: Frederico Araujo <frederico.araujo@ibm.com> * feat: cgo Go bindings (#45) * feat: initial revision rust core. Signed-off-by: Teryl Taylor <terylt@ibm.com> * fix: addressed comments in PR. Updated PluginContext to match spec. Signed-off-by: Teryl Taylor <terylt@ibm.com> * feat: added yaml and routing rule support. Signed-off-by: Teryl Taylor <terylt@ibm.com> * feat: added example code to show how to load manager and plugins. Signed-off-by: Teryl Taylor <terylt@ibm.com> * fixes: updated plugin errors, configs to more match python. Signed-off-by: Teryl Taylor <terylt@ibm.com> * feat: RUST CMF initial revision. Signed-off-by: Teryl Taylor <terylt@ibm.com> * feat: added invoke named support, added constants, fixed reviewed code. Signed-off-by: Teryl Taylor <terylt@ibm.com> * feat: added owned extensions and did some refactoring. Signed-off-by: Teryl Taylor <terylt@ibm.com> * feat: added cgo and golang bindings, examples and readme. Signed-off-by: Teryl Taylor <terylt@ibm.com> * address P0/P1/P2 review findings (except #17) Signed-off-by: Teryl Taylor <terylt@ibm.com> * fix: address remaining P2/P3 review findings + testing gaps Signed-off-by: Teryl Taylor <terylt@ibm.com> * docs: add CPEX Go public API spec Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com> * docs: renamed document Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com> * feat(cpex-rust): CGO review passes 1-11 + lint cleanup + Makefile targets Signed-off-by: Teryl Taylor <terylt@ibm.com> * fix: address linting issues, updated makefile to support building examples. Signed-off-by: Teryl Taylor <terylt@ibm.com> * docs: updated the go spec to reflect recent changes. Signed-off-by: Teryl Taylor <terylt@ibm.com> --------- Signed-off-by: Teryl Taylor <terylt@ibm.com> Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com> Co-authored-by: Teryl Taylor <terylt@ibm.com> Co-authored-by: Frederico Araujo <frederico.araujo@ibm.com> * docs: intial rust specification (#50) Co-authored-by: Teryl Taylor <terylt@ibm.com> * feat: change Plugin handler to async for performance (#49) Co-authored-by: Teryl Taylor <terylt@ibm.com> * fix: missing cmf-demo main.go file and gitignore fix that missed it (#52) Co-authored-by: Teryl Taylor <terylt@ibm.com> * feat: initial APL Rust implementation (#60) * fix: initial revision APL. * feat: apl-cpex bridge crate + plugin-registry-driven hook dispatch * feat: add support for plugin calling in APL routes. * feat: add more APL plugin support, unified config * feat: added cedar direct PDP. * feat: add identity hook and extensions. * feat: added token delegation hooks and tests. * feat: added plugin for jwt token identity, oauth and biscuit delegation, cedarling PDP. Signed-off-by: Teryl Taylor <terylt@ibm.com> * fix: updated identity and delegation to support keycloak. added delegate() function, and identity sections. * fix: added some sample plugins, added updates to support cedar. Signed-off-by: Teryl Taylor <terylt@ibm.com> * feat: added session support, serialize and parallel and full effects capabilities. * feat: add ffi pre-built .a library Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com> * chore: add workflow_dispatch target Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com> * fix: critical and high issues from review. * feat: add APL FFI and go bindings Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com> * chore: add musl tools to musl runners Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com> * fix: potential double free after use bug. * chore: update Go module paths after repo rename to cpex * feat: map identity extension into cpex ffi Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com> * feat: add cpex_invoke_resolved abi Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com> * fix: has_hook_for handling Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com> * chore: update headers Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com> --------- Signed-off-by: Teryl Taylor <terylt@ibm.com> Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com> Co-authored-by: Frederico Araujo <frederico.araujo@ibm.com> * fix: session binding Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com> * chore: updated comments Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com> * tests: added more session tests for Tier 1 ids. --------- Signed-off-by: Teryl Taylor <terylt@ibm.com> Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com> Co-authored-by: terylt <30874627+terylt@users.noreply.github.com> Co-authored-by: Teryl Taylor <terylt@ibm.com>
Summary
Phase 1a of the CPEX Rust plugin runtime. Introduces
cpex-coreandcpex-sdkcrates with a typed, 5-phase plugin execution framework.This is the foundation for multi-language plugin support (Go, Python, WASM) with a shared Rust core. The existing Python implementation in
cpex/framework/is unchanged and continues to work as-is.Closes: #15
What's included
Rust crates (
crates/)cpex-core— Core runtime (4,482 lines)Plugintrait (lifecycle: initialize, shutdown)HookTypeDeftrait (typed hook definitions with payload + result types)HookHandler<H>trait (borrow-based handlers — framework never clones)PluginResult<P>with separate payload and extension modificationsPluginPayloadtrait (object-safe base for all payloads)PluginRef(trusted config from loader — plugin can't tamper with priority/mode/capabilities)PluginRegistrywith typed registration and name-based multi-hook registration (CMF pattern)TypedHandlerAdapter(auto-bridges typed handlers to type-erased dispatch)Executor(Sequential → Transform → Audit → Concurrent → FireAndForget)PluginManagerwithregister_handler(),invoke::<H>(), andinvoke_by_name()PluginConfig,PluginMode,OnError,PluginCondition(backward compat)cpex-sdk— Lean re-exports for plugin authorsKey design decisions
fn handle(&self, payload: &P, ...). The framework holds payload ownership. Plugins clone only when modifying. Enforced by Rust's borrow checker at compile time.plugin.config(). The executor reads mode, priority, and capabilities fromPluginRef.trusted_config.invoke()path —AnyHookHandlerhas one method, not separate owned/ref paths. Simpler API.Box<dyn PluginPayload>instead ofBox<dyn Any>— type errors caught at compile time.HookHandler<CmfHook>impl registered for multiple hook names (cmf.tool_pre_invoke,cmf.llm_input, etc.).What's NOT included (future phases)
How to test