feat: add PluginResult.executions structured control execution records - #141
Conversation
Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
araujof
left a comment
There was a problem hiding this comment.
The construction of the records object looks good. One blocking issue I found is that the instrumentation changed enforcement behavior.
Critical
The try/except around execute_plugin in _run_serial_phase (manager.py:642) swallows the PluginError raised for on_error=fail and returns continue_processing=True. That's a fail-open issue: a control that errors or times out now allows the request instead of blocking it — in the default enforcement mode. Record the error, then re-raise (the ignore/disable cases are already handled). Add a fail-closed regression test; there isn't one, which is how this slipped through.
Non-critical
- Errors/timeouts are recorded as
COMPLETED— theexcept asyncio.TimeoutErrorbranch is dead, and a timeout underon_error=ignorelooks like a clean allow. - "One record per plugin, always present" is false: disabled/skipped plugins and cancelled concurrent siblings get no record, so
SKIPPED/DISABLED/CANCELLEDare never emitted. Wire them up or fix the docs. matchedisNoneon a concurrent clean allow butFalsein serial phases — same outcome, contradicts the docstring.- Fire-and-forget records say
COMPLETEDat spawn and are never reconciled. - Tests only cover the sequential path; transform/audit/concurrent/FAF record-building is untested, and the docstring claims FAF coverage that isn't there.
_truncateexceeds its 256-byte cap by 3 (the ellipsis); the test was loosened to match.
Minor
config_keys/kindrecomputed per plugin per hook though config is frozen — cache onPluginRef.ControlExecutionRecordbuilt at three drifting sites (already caused thematchedbug) — extract a factory.- Cleanups: redundant
except (PluginError, Exception), no-oprequested_allow = x if x else False, a tautological test assertion,__all__out of order.
Reusing the PluginResult to pass back the execution context (even though this could be passed in the metadata dictionary) is acceptable in 0.1.x. Let's revisit this in 0.2.x; I think we can leverage some of the work on security auditing that is being built in the Rust framework.
Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
|
Re-reviewed after The fix did introduce one regression in the concurrent phase. Fixed in Good to merge. |
Summary
Adds
PluginResult.executions— a list of structuredControlExecutionRecordobjects returned on everyinvoke_hookcall, giving consumers trusted per-plugin execution telemetry without parsing logs.Closes: #130
Changes
ControlExecutionStatusenum (6 variants:completed,error,timeout,cancelled,skipped,disabled) andControlExecutionRecordPydantic model (13 fields) tocpex/framework/models.pyexecutions: list[ControlExecutionRecord]field toPluginResult— always present, empty when no plugins ranPluginExecutor:duration_ns), derivesmatched/applied/effective_allowfrom result and mutation detectionduration_ns=0, no per-branch timing available)status=completed,duration_ns=0); identify FAF records bymode == "fire_and_forget"plugin_id,plugin_name,plugin_kind,mode) sourced fromPluginRef.trusted_config— plugins cannot forge themControlExecutionRecordandControlExecutionStatusfromcpex.frameworkCHANGELOG.mdplugin_idhex formatChecks
make lintpasses — note:rufffails onmax-statements-in-tryinpyproject.toml; this is a pre-existing issue on0.1.xunrelated to this PRmake testpasses — 1962 passed, 0 failedNotes
No breaking changes.
PluginResult.executionsis additive — existing CF call sites require zero changes.result.continue_processingandresult.violationare unchanged.Consumption pattern (CF side):