fix: violation record on exception - #148
Conversation
terylt
left a comment
There was a problem hiding this comment.
Hi @prakhar-singh1928 , Thanks for the PR. Overall it's good. Here are some suggested changes:
[Medium — incomplete fix] The same gap remains for CONCURRENT-mode plugins
execute_plugin raises PluginViolationError for both SEQUENTIAL and CONCURRENT modes
when violations_as_exceptions=True (manager.py:1177, :1194-1204). But the deny record
is only appended in _run_serial_phase. In the concurrent path:
execute_pluginraises inside the task;_taggedonly catchesPluginTimeoutError(manager.py:1028), so the PVE escapes the
as_completedloop before the record-append atmanager.py:451and before the
task-cancellation at:474;- the new outer handler then attaches an
executionslist that is missing the denying
concurrent plugin — and the pending sibling tasks are never cancelled (pre-existing
leak).
So a caller using violations_as_exceptions=True with any concurrent plugin still hits the
exact bug #147 describes. Recommend either (a) handling PVE in _tagged/the concurrent loop
symmetrically, or (b) explicitly scoping this PR to serial and noting the concurrent gap as
a follow-up.
Why it's serial-only: the record-writing code lives solely in
_run_serial_phase's exception handler. Concurrent plugins are dispatched in a separate
asyncio-task block that writes its record by inspecting a returned result
(manager.py:451). Because execute_plugin raises rather than returns for concurrent
denials under violations_as_exceptions=True, the exception unwinds past that append — that
concurrent-halt branch is effectively dead code in this mode.
[Low — traceability] Issue-number mismatch
The PR says "Closes #147", but every code comment, the errors.py docstring, and the test
section header reference cpex#130. #130 is the closed umbrella feature ("structured
control execution records"); #147 is the actual bug. The inline references should point at
#147.
[Nit] errors.py import is heavier than needed
errors.py already imports from cpex.framework.models at runtime
(PluginErrorModel, PluginViolation), so there's no circular-import risk in adding
ControlExecutionRecord to that existing line. The added from __future__ import annotations + TYPE_CHECKING guard works but is more machinery than required. Harmless
either way.
[Note — pre-existing] FAF records absent on the exception path
On a normal (non-exception) sequential halt, _build_halt_result schedules fire-and-forget
plugins and their records land in executions; on the violations_as_exceptions=True raise
path, FAF plugins don't fire, so pve.executions won't include them. Not introduced here,
but since the PR makes executions authoritative on the exception path, a one-line note
would keep consumers from assuming parity between the two halt shapes.
Test coverage
Good for the serial case. Gaps:
- No concurrent-mode test (which would surface the Medium issue above).
- No regression guard that the non-exception halt path still attaches
executions
correctly. - Consider asserting
pve.executionsis a distinct list from the manager's internal
accumulator (thelist(...)copy), so a later mutation can't leak back.
…onError
When violations_as_exceptions=True and a plugin denies an invocation,
the except PluginViolationError handler in _run_serial_phase previously
re-raised immediately without appending a ControlExecutionRecord for
the denying plugin to the executions accumulator. The same gap existed
for CONCURRENT-mode plugins: _tagged only caught PluginTimeoutError,
so PluginViolationError escaped the as_completed loop before the
record-append and before sibling task cancellation.
This meant PluginResult.executions only contained records for plugins
that ran *before* the denial, making it impossible for consumers to
identify which specific control blocked the invocation from telemetry
alone.
Changes:
- cpex/framework/manager.py (_run_serial_phase): Append the denying
plugin's ControlExecutionRecord (status=COMPLETED, effective_allow=False,
matched=True, applied=True, error_code/reason from violation) before
re-raising PluginViolationError. Covers SEQUENTIAL, TRANSFORM, AUDIT.
- cpex/framework/manager.py (_tagged): Catch PluginViolationError and
return it as a sentinel alongside the tag (mirrors PluginTimeoutError),
so it never escapes the concurrent as_completed loop unpaired.
- cpex/framework/manager.py (concurrent as_completed loop): Handle the
new violation_err sentinel — write the denial record, cancel all sibling
tasks, and re-raise. Fixes the pre-existing sibling-task leak on this
path.
- cpex/framework/manager.py (execute): Outer try/except PluginViolationError
attaches list(executions) to pve.executions before propagating. FAF
plugins are not scheduled on this raise path (noted in comment).
- cpex/framework/errors.py (PluginViolationError): Add executions attribute
(list[ControlExecutionRecord] | None, default None). Simplified import —
ControlExecutionRecord added to existing runtime import line, dropping
the unnecessary __future__/TYPE_CHECKING guard.
- tests/unit/cpex/framework/test_execution_records.py: Four new tests:
- test_executions_on_violation_exception_single_plugin
- test_executions_on_violation_exception_two_plugin_chain
- test_executions_on_violation_exception_concurrent_plugin (new — covers
the concurrent-mode gap)
- test_executions_on_violation_exception_list_is_independent_copy (new —
asserts pve.executions is a distinct list from the internal accumulator)
Fixes: #147
88ce985 to
fc51c0d
Compare
PRs targeting 0.1.x triggered no checks — every workflow filtered on main only. Add dev and 0.1.x, the long-lived maintenance base branch for the 0.1 series. Drop the docs job from the CI pipeline on this branch: docs are built and published from main only. It has to go from ci.yml rather than docs-build.yaml, because workflow_call bypasses the called workflow's own branch filter. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
ci.yml calls lint.yml and tests.yaml via workflow_call, but both also had their own push/pull_request triggers, so every job ran twice per PR. Make them reusable-only; ci.yml owns the branch filters. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
mcp 2.0.0 renamed McpError -> MCPError, breaking the import in cpex/framework/external/mcp/client.py and erroring out test collection. Verified McpError is present in 1.26.0 through 1.29.0 and absent in 2.0.0, so the cap is <2 rather than a tighter pin. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Clears the two E303 errors that failed the ruff job and the outstanding ruff format diff. Formatting only — ASTs are unchanged. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
|
Review feedback addressed:
Separately: CI had never run on this branch — every workflow filtered on |
Summary
Fixes the silent gap in
PluginResult.executionswhenviolations_as_exceptions=True— the denying plugin'sControlExecutionRecordwas never written, making it impossible to identify which control blocked an invocation from telemetry alone.Closes: #147
Changes
cpex/framework/manager.py—_run_serial_phase: Theexcept PluginViolationErrorblock now appends aControlExecutionRecordfor the denying plugin (status=COMPLETED,effective_allow=False,matched=True,applied=True,error_codeandreasonfrom the violation) before re-raising. Previously this path was a bareraisewith the comment "propagate immediately, no record needed".cpex/framework/manager.py—execute: The entire pipeline dispatch is now wrapped in atry/except PluginViolationErrorthat attacheslist(executions)topve.executionsbefore propagating. Callers catchingPluginViolationErrorcan now inspectpve.executionsto see the full execution chain including the blocking plugin.cpex/framework/errors.py—PluginViolationError: Addedexecutions: list[ControlExecutionRecord] | None = Noneattribute. Defaults toNone(backward compatible, no signature change). Set to the accumulated records list byexecute()before the exception reaches the caller.tests/unit/cpex/framework/test_execution_records.py: Two new tests directly assertpve.executionson the raised exception:test_executions_on_violation_exception_single_plugin— one plugin denies; asserts 1 record with correct fields.test_executions_on_violation_exception_two_plugin_chain— allow + deny chain; asserts 2 records in order, the second havingeffective_allow=Falseand the correcterror_code.Checks
make lintpassesmake testpassesNotes
No changes required on the ContextForge side. The existing
except PluginViolationErrorworkaround intool_service.py(ctl_acc.mark_denied(hook="pre/post")) continues to work unchanged —pve.executionsis purely additive.