Skip to content

feat: Python bindings (PyO3)#70

Open
tedhabeck wants to merge 11 commits into
devfrom
feat/cpex_python_bindings
Open

feat: Python bindings (PyO3)#70
tedhabeck wants to merge 11 commits into
devfrom
feat/cpex_python_bindings

Conversation

@tedhabeck

Copy link
Copy Markdown
Contributor

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::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)

Checks

  • make lint passes
  • make test passes

araujof and others added 5 commits June 12, 2026 10:26
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>
@tedhabeck tedhabeck marked this pull request as ready for review June 15, 2026 14:20
@araujof araujof changed the title [FEATURE]: Python bindings (PyO3) feat: Python bindings (PyO3) Jun 16, 2026

@terylt terylt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 crate cpex-python wrapping cpex_core::PluginManager via PyO3 0.29.
  • Python surface: PluginManager(config_path), initialize(), shutdown(), invoke_hook(...), plus a read-only PipelineResult.
  • Payloads cross as dicts (PyObject ↔ serde_json::Value ↔ typed). cmf.* hooks → MessagePayload; everything else → a local GenericPayload.
  • 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-python is in [workspace] members but not default-members, and every pure-Rust Makefile target uses --exclude cpex-python, so the workspace stays libpython-independent (KD3). The macOS build.rs -undefined dynamic_lookup shim is correct and well-commented.
  • GIL discipline is correct. PipelineResult stores serde_json::Value and builds PyDicts 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.md states 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 through GenericPayload in 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>() rejects u64 values above i64::MAX and bignums with an opaque OverflowError instead of the crate's cpex:-prefixed message. Real-world large IDs / timestamps could trip it. Consider a u64/f64 fallback or a typed error.
  • PipelineResult re-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 the PyDict on every access (reading .violation twice 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 testPY_WALL_CLOCK_TIMEOUT is never exercised.
  • No concurrent-invoke test — multiple invoke_hook calls in flight against the shared Arc<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 in builtins.rs and Cargo.toml flag a real maintenance smell: two hardcoded factory lists that must be kept aligned by hand. Fine for v1; worth a tracking note.
  • futures is listed as a dependency — verify it is actually used (possible unused dep; cargo-udeps / clippy would confirm).
  • The pii_deny.yaml fixture includes an audit_faf fire-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.

…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants