feat: Python bindings (PyO3)#70
Conversation
Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
…anager
Introduces `bindings/python/` — a new maturin-built Rust crate (`cpex-python`)
that wraps `cpex_core::PluginManager` via PyO3 0.29, exposing a native Python
`cpex` package backed entirely by the Rust runtime.
Key design points:
- `PluginManager(config_path)` — sync constructor; registers APL factories then
loads YAML config (order critical for APL Weak upgrade)
- `initialize()`, `shutdown()`, `invoke_hook()` — async via pyo3-async-runtimes
`future_into_py`; all guarded by a 60 s wall-clock timeout (PY_WALL_CLOCK_TIMEOUT)
- CMF hooks route payloads through `MessagePayload`; all others use a local
`GenericPayload<serde_json::Value>`
- `PipelineResult` exposes 7 read-only Python getters; `__repr__` is pointer-free
- `cpex-python` is in `[workspace] members` but not `default-members`, so all
pure-Rust CI targets remain libpython-independent (`--exclude cpex-python`)
- Makefile gains `bindings-python-{build,build-release,test}` targets
- 22 unit/integration tests; all pass alongside 2058 legacy tests (AE3)
Signed-off-by: habeck <habeck@users.noreply.github.com>
Signed-off-by: habeck <habeck@us.ibm.com>
terylt
left a comment
There was a problem hiding this comment.
PR Review — feat(bindings/python): PyO3 native bindings for cpex-core PluginManager
Commit: d158db5
Author: Ted Habeck
Scope reviewed: Ted's PR as committed (the v1 dict-based Python → Rust binding). Does not include later typed-wrapper work, which is a separate follow-up.
Verdict
Approve with comments. Solid, well-crafted, honestly scoped work. The code is idiomatic, the tests are thoughtful, the build isolation is deliberate, and the docs are honest about what is deferred rather than overclaiming. It does exactly what it sets out to: a v1 Python front door to the Rust manager running the Rust/APL builtins — dict in, dict out.
Two items should be resolved/verified before merge (CI coverage and panic safety). Everything else is a clean follow-up.
What the PR does (for reviewers new to it)
- Adds
bindings/python/— a maturin-built cratecpex-pythonwrappingcpex_core::PluginManagervia PyO3 0.29. - Python surface:
PluginManager(config_path),initialize(),shutdown(),invoke_hook(...), plus a read-onlyPipelineResult. - Payloads cross as dicts (
PyObject ↔ serde_json::Value ↔ typed).cmf.*hooks →MessagePayload; everything else → a localGenericPayload. - The plugins that run are the Rust builtins registered in
builtins.rs(validator/pii-scan,audit/logger,identity/jwt,delegator/oauth, APL visitor + Cedar PDP). It does not host Python plugins (that is the separate, unbuilt Rust → Python bridge).
Strengths
- Build isolation is done right.
cpex-pythonis in[workspace] membersbut notdefault-members, and every pure-Rust Makefile target uses--exclude cpex-python, so the workspace stays libpython-independent (KD3). The macOSbuild.rs-undefined dynamic_lookupshim is correct and well-commented. - GIL discipline is correct.
PipelineResultstoresserde_json::Valueand buildsPyDicts lazily in the getters — specifically so the async result-building block never needs the GIL. A deliberate, correct design choice, not just laziness. - Honest docs.
MIGRATION.mdstates plainly that this is a break from the legacy package (dict not pydantic, single result not a 2-tuple, separate venv required) and lists the v2 deferrals (typed identity/delegation route throughGenericPayloadin v1). No drop-in overclaim. - Thoughtful tests. Depth 128 vs 129 boundary, non-string keys, unconvertible types, pointer-free
repr, malformed YAML, invalid CMF payload, deny-result shape. Genuinely good edge coverage for the conversion layer.
Findings
🔴 Verify before merge — CI coverage
The binding is excluded from rust-build / rust-test / clippy (all use --exclude cpex-python). The only thing that builds and tests it is the bindings-python-test Makefile target, which requires maturin.
Action: confirm CI actually invokes make bindings-python-test (or equivalent). If CI runs only the pure-Rust targets, Ted's 22 tests never execute in CI and the binding can silently rot against cpex-core changes. This is a process gap, not a code defect — but it is the most important thing to settle before merge.
🟠 Should-fix — panic safety
invoke_hook relies on the 60 s wall-clock timeout but does not wrap invoke_by_name in catch_unwind. The FFI crate deliberately does (run_safely). A panicking plugin may therefore unwind across the await and crash the Python process instead of surfacing as a Python exception. The in-code comment acknowledges panics "propagate as task aborts" but does not convert them to a PyErr.
Action: confirm whether pyo3_async_runtimes::tokio::future_into_py catches panics. If it does not, mirror the FFI crate's catch_unwind so a panicking plugin becomes a RuntimeError. No test currently exercises a panicking plugin.
🟡 Nice-to-have
i64-only integer conversion (bindings/python/src/conversions.rs,pyobj_to_json_value):i.extract::<i64>()rejectsu64values abovei64::MAXand bignums with an opaqueOverflowErrorinstead of the crate'scpex:-prefixed message. Real-world large IDs / timestamps could trip it. Consider au64/f64fallback or a typed error.PipelineResultre-converts per getter access (bindings/python/src/result.rs): every field is eagerly serialized in the builder even if never read, and each getter rebuilds thePyDicton every access (reading.violationtwice does the work twice). This is the tradeoff for the GIL-correct design above, so it is defensible — a// TODO(perf)would be fair.- No timeout test —
PY_WALL_CLOCK_TIMEOUTis never exercised. - No concurrent-invoke test — multiple
invoke_hookcalls in flight against the sharedArc<PluginManager>; worth one test given the gateway use case. - No modified-payload round-trip test — only the deny path is asserted; a transform plugin actually mutating the payload is not covered.
⚪ Nits
- The "Keep in sync with
cpex-ffi" comments inbuiltins.rsandCargo.tomlflag a real maintenance smell: two hardcoded factory lists that must be kept aligned by hand. Fine for v1; worth a tracking note. futuresis listed as a dependency — verify it is actually used (possible unused dep;cargo-udeps/ clippy would confirm).- The
pii_deny.yamlfixture includes anaudit_faffire-and-forget plugin, but no test asserts its side effect post-shutdown()— the KD4 drain behavior is claimed but not directly tested.
Scope note (not a defect)
The identity/delegation hooks routing through GenericPayload (rather than typed IdentityPayload / DelegationPayload) is intentional and documented as a v2 deferral in MIGRATION.md. It is not a bug. Any typed-identity/delegation work — input routing in resolve_payload and the matching serialize_payload arms — belongs in the v2 follow-up, not retrofitted into this PR.
Recommendation
Approve with comments. Pre-merge: (1) confirm CI exercises the binding, and (2) resolve panic safety (verify or mirror the FFI catch_unwind). The remaining items are clean follow-ups. This is genuinely good work, correctly scoped, with documented deferrals.
Signed-off-by: habeck <habeck@us.ibm.com>
…dated Cargo.toml and builtins.rs to use the new cpex-builtins facade, then resolved two clippy lint failures. Signed-off-by: habeck <habeck@us.ibm.com>
Signed-off-by: habeck <habeck@us.ibm.com>
Signed-off-by: habeck <habeck@us.ibm.com>
…ngs. Signed-off-by: habeck <habeck@us.ibm.com>
Summary
Added PyO3 native Python bindings for cpex-core.
Closes: #19
Changes
Introduces
bindings/python/— a new maturin-built Rust crate (cpex-python)that wraps
cpex_core::PluginManagervia PyO3 0.29, exposing a native Pythoncpexpackage backed entirely by the Rust runtime.Key design points:
PluginManager(config_path)— sync constructor; registers APL factories thenloads YAML config (order critical for APL Weak upgrade)
initialize(),shutdown(),invoke_hook()— async via pyo3-async-runtimesfuture_into_py; all guarded by a 60 s wall-clock timeout (PY_WALL_CLOCK_TIMEOUT)MessagePayload; all others use a localGenericPayload<serde_json::Value>PipelineResultexposes 7 read-only Python getters;__repr__is pointer-freecpex-pythonis in[workspace] membersbut notdefault-members, so allpure-Rust CI targets remain libpython-independent (
--exclude cpex-python)bindings-python-{build,build-release,test}targetsChecks
make lintpassesmake testpasses